Shireesh Anjal 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();
I believe cluster name is available automatically, and a command doesn't need
to add that explicitly inside getJobMessageProperties(). Please correct me if
wrong.
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());
ok - will add protection here..
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:
rubBllAction() is present in base class, and it sets the status of current
command based on result of the internal command executed. I think it makes
sense, and makes the code in this class simpler.
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: *
I think they are self explanatory and don't really need a javadoc. I don't mind
removing @param and @return completely if you insist.
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: */
I would like to keep it, unless you have a *strong* objection to it.
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: */
"you propagate only the response code but not the reason of it as it is done
explicitly where this method is called from."
-> I don't understand. What exactly do you mean by reason of a response code?
As I mentioned earlier, I believe this makes code simpler in derived classes,
and I would like to keep this method, unless this is a blocker for you to
approve the patch.
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);
ok - assuming that such a method exists the base class.
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
ok, will do.
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]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
