Allon Mureinik has posted comments on this change.
Change subject: core: Allow force re-election of a specific host as SPM
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
As far as I get it, you're basically sending a StopSPM command with an
additional parameter, the ID of the host you want to use for the SPM.
If it succeeds - all is well.
If it doesn't, we try the next host. I'm not sure what the semantics of sending
host1 and getting a new SPM on host2.
Should this be marked as a success?
Giving -1 to indicate this question should be discussed (possibly on
engine-devel, as Michael suggests).
(Also, some nitpicking inline)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ReinitializeSPMCommand.java
Line 14: import org.ovirt.engine.core.dal.VdcBllMessages;
Line 15: import
org.ovirt.engine.core.vdsbroker.irsbroker.SpmStopOnIrsVDSCommandParameters;
Line 16:
Line 17: @NonTransactiveCommandAttribute
Line 18: public class ReinitializeSPMCommand<T extends
ReinitializeSPMParameters> extends CommandBase<T> {
What about locking?
Should we?
Line 19:
Line 20: private static final long serialVersionUID = -4441933756184102371L;
Line 21:
Line 22: public ReinitializeSPMCommand(T parameters) {
Line 24: }
Line 25:
Line 26: @Override
Line 27: protected boolean canDoAction() {
Line 28: setVdsId(getParameters().getPrefferedSPMId());
Should be done in the constructor
Line 29:
Line 30: if (getVds() == null) {
Line 31: return failCanDoAction(VdcBllMessages.VDS_NOT_EXIST);
Line 32: }
Line 41:
Line 42: if (getVds().getVdsSpmPriority() ==
BusinessEntitiesDefinitions.HOST_MIN_SPM_PRIORITY) {
Line 43: return
failCanDo(VdcBllMessages.CANNOT_REINIT_SPM_VDS_MARKED_AS_NEVER_SPM);
Line 44: }
Line 45:
To be on the safe side, you should probably also make sure it's part of the DC
in the parameters
Line 46: return true;
Line 47: }
Line 48:
Line 49: private boolean failCanDo(VdcBllMessages message) {
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/IrsBrokerCommand.java
Line 134: private final String storagePoolRefreshJobId;
Line 135: private final java.util.HashSet<Guid> mTriedVdssList = new
java.util.HashSet<Guid>();
Line 136: private Guid mCurrentVdsId;
Line 137:
Line 138: private Guid prefferedHost;
Consider renaming this member (and its getter/setter) to preferredHostId
Line 139:
Line 140: public Guid getCurrentVdsId() {
Line 141: return getIrsProxy() != null ? mCurrentVdsId : Guid.Empty;
Line 142: }
Line 188: }
Line 189:
Line 190: }
Line 191: }
Line 192: } catch (Exception ex) {
empty blocks are evil.
If you already touched it, consider adding a comment that explains why this is
OK
Line 193: }
Line 194: }
Line 195:
Line 196: private int _errorAttempts;
--
To view, visit http://gerrit.ovirt.org/12489
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife292b85270e6aa8f5bf723ad7d3fa84e325d3d8
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
