Ramesh N has posted comments on this change. Change subject: gluster: Support for updating gluster volume and brick ......................................................................
Patch Set 2: (5 comments) http://gerrit.ovirt.org/#/c/25716/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/UpdateGlusterVolumeBricksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/UpdateGlusterVolumeBricksCommand.java: Line 15: /** Line 16: * BLL command to update gluster volume bricks Line 17: */ Line 18: @NonTransactiveCommandAttribute Line 19: @LockIdNameAttribute(isWait = true) > Lock on volume or cluster? I think lock at volume itself is enough. Also we are not going to gluster to update the status. Its only DB change in engine. So I will implement the lock for volume. Line 20: public class UpdateGlusterVolumeBricksCommand extends GlusterVolumeCommandBase<GlusterVolumeBricksParameters> { Line 21: Line 22: public UpdateGlusterVolumeBricksCommand(GlusterVolumeBricksParameters params) { Line 23: super(params); Line 62: } Line 63: Line 64: @Override Line 65: public AuditLogType getAuditLogTypeValue() { Line 66: if (getSucceeded()) { > You may want to pass custom variable to audit log type for reason of update Done Line 67: return AuditLogType.GLUSTER_VOLUME_BRICK_UPDATE; Line 68: } else { Line 69: return errorType == null ? AuditLogType.GLUSTER_VOLUME_BRICK_UPDATE_FAILED : errorType; Line 70: } http://gerrit.ovirt.org/#/c/25716/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/UpdateGlusterVolumeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/UpdateGlusterVolumeCommand.java: Line 11: /** Line 12: * BLL command to update a Gluster volume Line 13: */ Line 14: @NonTransactiveCommandAttribute Line 15: @LockIdNameAttribute(isWait = true) > Lock the volume or the cluster? Will lock the volume not the cluster. Line 16: public class UpdateGlusterVolumeCommand extends GlusterVolumeCommandBase<UpdateGlusterVolumeParameters> { Line 17: Line 18: public UpdateGlusterVolumeCommand(UpdateGlusterVolumeParameters params) { Line 19: super(params); Line 32: Line 33: @Override Line 34: protected void executeCommand() { Line 35: GlusterVolumeEntity glusterVolumeEntity = getParameters().getVolume(); Line 36: GlusterDBUtils.getInstance().updateVolumeStatus(glusterVolumeEntity.getId(), > Shouldn't the log message also log the reason for update? Done Line 37: glusterVolumeEntity.getStatus()); Line 38: setSucceeded(true); Line 39: } Line 40: http://gerrit.ovirt.org/#/c/25716/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/UpdateGlusterVolumeParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/UpdateGlusterVolumeParameters.java: Line 30: } Line 31: Line 32: public void setVolume(GlusterVolumeEntity volume) { Line 33: this.volume = volume; Line 34: } > I think we should probably have a reason field as well that could be used i Done -- To view, visit http://gerrit.ovirt.org/25716 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I244f9023924deaf5afea1dc3d12f7be9087a8cd1 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@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
