Moti Asayag has posted comments on this change. Change subject: engine : Remove Disk command using getVolumeInfo ......................................................................
Patch Set 3: (4 comments) https://gerrit.ovirt.org/#/c/39374/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java: Line 292: @Override Line 293: protected void executeCommand() { Line 294: switch (getDisk().getDiskStorageType()) { Line 295: case IMAGE: Line 296: if (getDisk().getDiskStorageType() == DiskStorageType.IMAGE) { please consider extracting the entire block into a method. Line 297: Future<VdcReturnValueBase> future = CommandCoordinatorUtil.executeAsyncCommand( Line 298: VdcActionType.RemoveImage, Line 299: buildRemoveImageParameters(getDiskImage()), Line 300: cloneContextAndDetachFromParent()); Line 298: VdcActionType.RemoveImage, Line 299: buildRemoveImageParameters(getDiskImage()), Line 300: cloneContextAndDetachFromParent()); Line 301: try { Line 302: VdcReturnValueBase vdcReturnValue = future.get(); would this call for future.get() will block this command's thread until the callback is completed ? Line 303: if (vdcReturnValue.getSucceeded()) { Line 304: incrementVmsGeneration(); Line 305: setSucceeded(true); Line 306: } https://gerrit.ovirt.org/#/c/39374/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveImageCommandCallback.java: Line 9: import org.ovirt.engine.core.compat.Guid; Line 10: import org.ovirt.engine.core.vdsbroker.irsbroker.IrsProxyData; Line 11: import org.ovirt.engine.core.vdsbroker.irsbroker.OneImageInfoReturnForXmlRpc; Line 12: Line 13: import java.util.List; the eclipse formatter orders the imports in a lexicographic order Line 14: Line 15: public class RemoveImageCommandCallback extends CommandCallback { Line 16: Line 17: private OneImageInfoReturnForXmlRpc imageInfoReturn; Line 39: VdcBllErrors bllErrors = VdcBllErrors.forValue(imageInfoReturn.getXmlRpcStatus().mCode); Line 40: if (bllErrors == VdcBllErrors.ImagePathError) { Line 41: cmd.setCommandStatus(CommandStatus.SUCCEEDED); Line 42: } else { Line 43: imageInfoReturn = null; I'm a bit troubled about the structure of this callback: inside the doPolling() method there are 3 steps: 1. initialize the callback (in this case the imageInfoReturn and irsProxyData). 2. execute the logic 3. in case of a failure, reset the class so it can re-polled. We should avoid instance-level parameters, and rescope them into this method. if there is a good reason for having those at instance level, perhaps we can have prePolling() and postPolling() . it is a common practice. Line 44: } Line 45: } Line 46: } Line 47: -- To view, visit https://gerrit.ovirt.org/39374 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I458143387f89a5d55bda5f1d4f9d44be4f7fb197 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
