anmolbabu has posted comments on this change. Change subject: engine : Refactoring in accordance with vdsm patches ......................................................................
Patch Set 5: (3 comments) http://gerrit.ovirt.org/#/c/37300/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/PauseGlusterVolumeGeoRepSessionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/PauseGlusterVolumeGeoRepSessionCommand.java: Line 25: protected boolean canDoAction() { Line 26: if (!super.canDoAction()) Line 27: return false; Line 28: if (getGeoRepSession().getStatus() == GeoRepSessionStatus.PASSIVE) { Line 29: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_GEOREP_SESSION_ALREADY_PAUSED); > why not return failCanDoAction? Well I just checked the behavioual difference b/w the two functions and found that both of them are same except for the case where we pass a variable length variable replacements which we aren't doing. Anyways, this has come in an attempt to fix the error of AuditLogMessages not appearing in the events tab, and as part of fixing this I literally started comparing the skeletons of the working Resume command and the other non-working ones but ultimately found the culprit to be mismatch b/w the entry in AuditLogType and AuditLogMessages. As I don't really find any difference in behaviour of addCanDoActionMessage and failCanDoAction for our case except for the fact that failCanDoAction itself returns the failure boolean. I strongly agree that this change is not necessary. Line 30: return false; Line 31: } Line 32: return true; Line 33: } Line 50: } Line 51: Line 52: @Override Line 53: public AuditLogType getAuditLogTypeValue() { Line 54: if(getSucceeded()) > Did you apply a different formatting style in Eclipse? I think I enabled : Remove unnecessary blocks (some thing of this kind) in my eclipse save preference I'll undo this. Line 55: return AuditLogType.GLUSTER_VOLUME_GEO_REP_PAUSE; Line 56: else Line 57: return AuditLogType.GLUSTER_VOLUME_GEO_REP_PAUSE_FAILED; Line 58: } http://gerrit.ovirt.org/#/c/37300/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/StartGlusterVolumeGeoRepCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/StartGlusterVolumeGeoRepCommand.java: Line 24: @Override Line 25: protected void executeCommand() { Line 26: GlusterGeoRepSession session = getGeoRepSession(); Line 27: VDSReturnValue returnValue = runVdsCommand(VDSCommandType.StartGlusterVolumeGeoRep, new GlusterVolumeGeoRepSessionVDSParameters(upServer.getId(), getGlusterVolumeName(), session.getSlaveHostName(), session.getSlaveVolumeName())); Line 28: setSucceeded(returnValue.getSucceeded()); > Shouldn't you set the status to Initialising? What would it be by default? Initialising if the session is non faulty not sure in faulty cases I guess it is mostly Initialising bcoz I haven't myself seen any other states but thought it be safe to leave it to the sync job may be wholly influenced by a discussion in one of our demos. Line 29: if (!getSucceeded()) { Line 30: handleVdsError(AuditLogType.GLUSTER_VOLUME_GEO_REP_START_FAILED_EXCEPTION, returnValue.getVdsError().getMessage()); Line 31: return; Line 32: } -- To view, visit http://gerrit.ovirt.org/37300 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I702d6c62f576cf2a49437fcf37155290d8580588 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
