Ayal Baron has posted comments on this change.
Change subject: core: Use the hostSpmId during reconstructMaster
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java
Line 43: DeactivateStorageDomainCommand<T> {
ReconstructMasterDomain extends DeactivateStorageDomain, that means that
reconstruct is a private case of deactivation.... ??????????????????????????
Line 76: protected void executeCommand() {
This function is nearly 100 loc long.
Adding more lines to it does not make sense.
This needs URGENT refactoring *before* any new logic is introduced.
Due to this fact, I am commenting on the entire function even though the patch
deals with a very small subset and most of the code predates it.
Line 80: @Override
A good place to start:
at least 2 redundant levels of indentation here (makes reading the function
more difficult)
Line 87: if (getParameters().getIsDeactivate()) {
getIs takes an adjective not a verb (i.e. should be getIsInactive but I don't
think that's in the scope even if refactoring)
Most likely that this piece of code has no business here, but I assume that
this is due to the decision to inherit reconstruct from deactivate. I have no
clue as to why I would do this if I already called reconstruct (to me it seems
that this should have been a prerequisite of this command)
Line 93: if (!_isLastMaster) {
another redundant indentation.
Just do:
commandSucceeded = stopSPM() // This is fine since both code paths do this
first thing !?!
if (isLastMaster) {
return commandSucceeded;
}
then the next *50* loc are easier to read.
Line 94: // pause the timers for the domain
error handling.
why are we pausing the timers?
Line 102: commandSucceeded = stopSpm();
there are 2 accepted patterns for failing when having an error:
1. the function throws/raises an exception
2. the function returns an error and the caller checks and handles the error.
In this case:
if (!stopSpm()) {
return false;
}
And as stated above, should probably be moved out of the if clause.
Dragging the failure all over the code just makes everything cumbersome.
Line 103: if (!Config.<Boolean>
GetValue(ConfigValues.ReconstructMasterHostId,
ReconstructMasterHostId does not convey the fact that disconnect is not needed
here. nor does it convey the fact that we need to pass the new param.
It should probably be something like: SANLockDomain
Which leads me to the (mis?)conclusion that this test is wrong as it implies
that 3.1 dc level (I hope this is about dc level, I can't figure that one out
from getcompatibility_version) means V3 storage domain (which I hope will be
true, but what you really want to check is the storage domain version).
Line 116:
.getSucceeded();
if (!commandSucceeded) {
return false;
}
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ReconstructMasterVDSCommandParameters.java
Line 9: private int privateVdsSpmId;
Federico, this is not a question to you.
I can see that this is the convention, but why does one need the 'private'
prefix?
The compiler enforces this and all access to it is done through a getter and
setter anyway...
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ReconstructMasterVDSCommand.java
Line 41: status =
getBroker().reconstructMaster(getParameters().getStoragePoolId().toString(),
if VdsSpmId were passed as the last argument instead of in the middle then you
could always send it as old VDSMs would treat at as 'options' and just ignore
it while the newer VDSM's would treat it properly and use it.
Then you wouldn't need all these changes.
--
To view, visit http://gerrit.ovirt.org/4967
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8c4fceeda898e020a4153ccd36aa078eb200ac3
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Livnat Peer <lp...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
