Shubhendu Tripathi has posted comments on this change. Change subject: gluster: BLL command for volume snapshot create ......................................................................
Patch Set 9: (9 comments) http://gerrit.ovirt.org/#/c/34928/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeSnapshotCommand.java: Line 21: import org.ovirt.engine.core.common.vdscommands.VDSCommandType; Line 22: import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; Line 23: import org.ovirt.engine.core.common.vdscommands.gluster.GlusterVolumeCreateSnapshotVDSParameters; Line 24: Line 25: public class CreateGlusterVolumeSnapshotCommand extends GlusterCommandBase<CreateGlusterVolumeSnapshotParameters> { > extend GlusterCommandBase or GlusterVolumeCommandBase ? I remember the whole entity object is passed as parameter and so this way. Will check the feasibility to change as suggested, and if possible would change it. Line 26: Line 27: private GlusterVolumeSnapshotEntity snapshot; Line 28: private boolean force; Line 29: Line 61: Line 62: if (!getSucceeded()) { Line 63: handleVdsError(AuditLogType.GLUSTER_VOLUME_SNAPSHOT_CREATE_FAILED, retVal.getVdsError().getMessage()); Line 64: } else { Line 65: GlusterVolumeSnapshotEntity createdSnapshot = (GlusterVolumeSnapshotEntity)(retVal.getReturnValue()); > Why not just do snapshot.setId() with return value (which should return the yes. will change it Line 66: createdSnapshot.setClusterId(volume.getClusterId()); Line 67: createdSnapshot.setVolumeId(volume.getId()); Line 68: createdSnapshot.setDescription(snapshot.getDescription()); Line 69: createdSnapshot.setSnapshotName(snapshot.getSnapshotName()); Line 70: createdSnapshot.setCreatedAt(new Date()); Line 71: createdSnapshot.setStatus(GlusterSnapshotStatus.ACTIVATED); Line 72: getDbFacade().getGlusterVolumeSnapshotDao().save(createdSnapshot); Line 73: } Line 74: addCustomValue(GlusterConstants.VOLUME_SNAPSHOT_NAME, getParameters().getSnapshot().getSnapshotName()); > This can be set in the setActionMessageParameters itself? yes. will do that Line 75: } Line 76: Line 77: @Override Line 78: protected boolean canDoAction() { Line 90: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_DOES_NOT_SUPPORT_GLUSTER); Line 91: return false; Line 92: } Line 93: Line 94: GlusterVolumeEntity volume = getDbFacade().getGlusterVolumeDao().getById(snapshot.getVolumeId()); > How about check that snapshot.getVolumeId is not null? yes. will change Line 95: if (volume.getStatus() == GlusterStatus.DOWN) { Line 96: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_IS_DOWN); Line 97: } Line 98: Line 91: return false; Line 92: } Line 93: Line 94: GlusterVolumeEntity volume = getDbFacade().getGlusterVolumeDao().getById(snapshot.getVolumeId()); Line 95: if (volume.getStatus() == GlusterStatus.DOWN) { > possible NPE? You could probably setGlusterVolumeId in constructor and use will do that Line 96: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_IS_DOWN); Line 97: } Line 98: Line 99: if (!isVolumeThinlyProvisioned(volume)) { Line 102: Line 103: return true; Line 104: } Line 105: Line 106: private boolean isVolumeThinlyProvisioned(GlusterVolumeEntity volume) { > Please add a TODO comment here...on how this will be implemented done Line 107: return true; Line 108: } Line 109: Line 110: @Override Line 108: } Line 109: Line 110: @Override Line 111: protected Map<String, Pair<String, String>> getExclusiveLocks() { Line 112: GlusterVolumeEntity volume = getDbFacade().getGlusterVolumeDao().getById(snapshot.getVolumeId()); > volume object is not required here, as you're only using id. done Line 113: if (!isInternalExecution()) { Line 114: return Collections.singletonMap(volume.getId().toString(), Line 115: LockMessagesMatchUtil.makeLockingPair(LockingGroup.GLUSTER, VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); Line 116: } http://gerrit.ovirt.org/#/c/34928/9/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/CreateGlusterVolumeSnapshotParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/CreateGlusterVolumeSnapshotParameters.java: Line 14: private GlusterVolumeSnapshotEntity snapshot; Line 15: Line 16: private boolean force; Line 17: Line 18: private boolean createWithGeoRep; > What's this flag used for? We are planning to introduce a confirmation dialog if geo-rep is running for the volume. IF user selects Yes, snapshot creation is triggered else not. May be from UI itself we can stop and then this flag would not be required. Will check that out and do the necessary changes. Line 19: Line 20: public CreateGlusterVolumeSnapshotParameters() { Line 21: } Line 22: http://gerrit.ovirt.org/#/c/34928/9/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 108: VAR__ACTION__REMOVE_BRICKS_COMMIT, Line 109: VAR__ACTION__HOT_SET_CPUS, Line 110: VAR__ACTION__UPDATE_SLA_POLICY, Line 111: VAR__ACTION__UPDATE_VM_VERSION, Line 112: VAR__ACTION__VOLUME_SNAPSHOT_CREATE, > Do we need a separate action type - wouldn't VAR__ACTION__CREATE suffice? yes. we can use that. Will remove this. Line 113: Line 114: // Host statuses replacements Line 115: VAR__HOST_STATUS__UP, Line 116: VAR__HOST_STATUS__UP_MAINTENANCE_OR_NON_OPERATIONAL, -- To view, visit http://gerrit.ovirt.org/34928 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c27f9e73bac8d6de3022b0dd4e70aa0d8b1494b Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
