Moti Asayag has posted comments on this change.
Change subject: gluster: Make set option a step of create volume
......................................................................
Patch Set 1: (5 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();
Indeed the cluster name is added, but the way it is being added isn't optimal.
Resolving its name relies on the MLA while it can be obtained directly.
I'll send a patch for that.
Line 56: jobProperties.put(GlusterConstants.VOLUME,
getParameters().getVolume().getName());
Line 57: }
Line 58: return jobProperties;
Line 59: }
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:
But in any case the result of the invoked command is being analysed - in this
case it is even less clear since you refer to "getSucceeded()" of the current
command which was changed by a method (runBllAction) in a non sequencial flow.
I think it is much simpler to have code that does:
VdcReturnValueBase setOptionReturnValue =
getBackend().runInternalAction(VdcActionType.SetGlusterVolumeOption,
new GlusterVolumeOptionParameters(option),
createCommandContext(volume, option));
if (!setOptionReturnValue.getSucceeded()) {
setSucceeded(false);
....
}
It is less line of codes and less 2 methods to maintain.
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: *
Please remove so it won't look incomplete.
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 104: * @param actionType
Line 105: * @param params
Line 106: * @param context
Line 107: * @return
Line 108: */
This method modifies the status of the calling command without setting its
failure reason.
Instead the result is analysed for the failure reason and set them to the
caller method.
I don't think that it ease the reading.
It makes the flow of the program somehow complicated:
Before invoking runBllAction() the caller method had certain status and after
calling it, the status is changed to failure.
But in any case you evaluate the result of this command, so why let it decide
that it should fail the caller command ?
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);
yes - see CommandBase.getBackend()
Line 111: setSucceeded(returnValue.getSucceeded());
Line 112: return returnValue;
Line 113: }
Line 114:
--
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]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
