Sahina Bose 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 ? 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 Guid) and save 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? 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? 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 getGlusterVolume here 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 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. And as per previous comment, if you set the volume id in constructor, you can use getGlusterVolumeId() 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? 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? 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: 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
