Moti Asayag has posted comments on this change.
Change subject: gluster: Make set option a step of create volume
......................................................................
Patch Set 1: (8 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeCommand.java
Line 51:
Line 52: @Override
Line 53: public Map<String, String> getJobMessageProperties() {
Line 54: if (jobProperties == null) {
Line 55: jobProperties = super.getJobMessageProperties();
there is no default implementation of this method on its super - i think that
it would be better adding such implementation to GlusterCommandBase to ease the
code reading and performs the precise action you wish it to (in this case
adding the cluster name to the map by querying the vds_groups table).
Note that this is not part of the current patch and could be added later
without affecting this patch.
Line 56: jobProperties.put(GlusterConstants.VOLUME,
getParameters().getVolume().getName());
Line 57: }
Line 58: return jobProperties;
Line 59: }
Line 52: @Override
Line 53: public Map<String, String> getJobMessageProperties() {
Line 54: if (jobProperties == null) {
Line 55: jobProperties = super.getJobMessageProperties();
Line 56: jobProperties.put(GlusterConstants.VOLUME,
getParameters().getVolume().getName());
please note that his method is being called prior to parameters validation,
therefore the code should be protective as it goes to non-valid parameters
(verify getParameters().getVolume() is not null before accessing it)
Line 57: }
Line 58: return jobProperties;
Line 59: }
Line 60:
Line 184: List<String> errors = new ArrayList<String>();
Line 185: for (GlusterVolumeOptionEntity option : volume.getOptions()) {
Line 186: // make sure that volume id is set
Line 187: option.setVolumeId(volume.getId());
Line 188:
although the runBllAction was introduced in earlier patch, i fill the sequence
of this method is not clear: you perform runBllAction() and somehow the current
command status is updated. I think referring the return value of the command
here will be clearer to understand.
Line 189: VdcReturnValueBase setOptionReturnValue =
Line 190: runBllAction(
Line 191: VdcActionType.SetGlusterVolumeOption,
Line 192: new GlusterVolumeOptionParameters(option),
Line 203: }
Line 204:
Line 205: /**
Line 206: * Creates command context for setting a given option on the
given volume
Line 207: *
it would be nice to complete the rest of the javadoc (parameter and return
value)
Line 208: * @param volume
Line 209: * @param option
Line 210: * @return
Line 211: */
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterCommandBase.java
Line 90: *
Line 91: * @param actionType
Line 92: * @param params
Line 93: * @return
Line 94: */
with this patch there is no use for this method and i'd consider removing it as
well as explained below
Line 95: protected VdcReturnValueBase runBllAction(VdcActionType
actionType, VdcActionParametersBase params) {
Line 96: VdcReturnValueBase returnValue =
Backend.getInstance().runInternalAction(actionType, params);
Line 97: setSucceeded(returnValue.getSucceeded());
Line 98: return returnValue;
Line 104: * @param actionType
Line 105: * @param params
Line 106: * @param context
Line 107: * @return
Line 108: */
this method feels somewhat partial - i'd consider not using it, as it performs
only partial logic - since you propagate only the response code but not the
reason of it as it is done explicitly where this method is called from.
In addition, it makes the sequence of invocation not clear.
I'd call getBackend().runInternalAction(actionType, params, context) and decide
how to act upon its response.
Line 109: protected VdcReturnValueBase runBllAction(VdcActionType
actionType, VdcActionParametersBase params, CommandContext context) {
Line 110: VdcReturnValueBase returnValue =
Backend.getInstance().runInternalAction(actionType, params, context);
Line 111: setSucceeded(returnValue.getSucceeded());
Line 112: return returnValue;
Line 106: * @param context
Line 107: * @return
Line 108: */
Line 109: protected VdcReturnValueBase runBllAction(VdcActionType
actionType, VdcActionParametersBase params, CommandContext context) {
Line 110: VdcReturnValueBase returnValue =
Backend.getInstance().runInternalAction(actionType, params, context);
please use getBackend() instead of Backend.getInstance()
Line 111: setSucceeded(returnValue.getSucceeded());
Line 112: return returnValue;
Line 113: }
Line 114:
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties
Line 101: step.CREATING_SNAPSHOTS=Creating snapshots for VM ${VM}
Line 102: step.RUN_STATELESS_VM=Running stateless VM ${VM}
Line 103: step.TAKING_VM_FROM_POOL=Taking VM ${VM} from VM pool in order to run
it
Line 104: step.CLONE_IMAGE_STRUCTURE=Cloning image's structure
Line 105: step.SYNC_IMAGE_DATA=Synchronizing image data
please add a space line so it would be more noticeable a new section begins
Line 106: # Gluster step types
Line 107: step.SETTING_GLUSTER_OPTION=Setting option ${Key}=${Value} on volume
${GlusterVolume} of cluster ${Cluster}
Line 108:
Line 109: # Non-monitored job:
--
To view, visit http://gerrit.ovirt.org/10905
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0f22e9862584f1a616fcc01e8a80b7d5a5ffc78
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
