Daniel Erez has posted comments on this change. Change subject: core: Introduce restore from Cinder snapshot ......................................................................
Patch Set 3: (6 comments) https://gerrit.ovirt.org/#/c/42337/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RestoreFromCinderSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RestoreFromCinderSnapshotCommand.java: Line 24: */ Line 25: @InternalCommandAttribute Line 26: public class RestoreFromCinderSnapshotCommand<T extends RestoreFromSnapshotParameters> extends BaseImagesCommand<T> { Line 27: Line 28: private final ArrayList<Guid> _imagesToDelete = new ArrayList<>(); why '_' prefix? can be declared as List? Line 29: Line 30: public RestoreFromCinderSnapshotCommand(T parameters, CommandContext cmdContext) { Line 31: super(parameters, cmdContext); Line 32: } Line 54: case STATELESS: Line 55: if (imageToRemoveId != null) { Line 56: removeSnapshot(getDiskImageDao().get(imageToRemoveId)); Line 57: } Line 58: redundant Line 59: break; Line 60: } Line 61: Line 62: VDSReturnValue vdsReturnValue = performImageVdsmOperation(); Line 58: Line 59: break; Line 60: } Line 61: Line 62: VDSReturnValue vdsReturnValue = performImageVdsmOperation(); why do we invoke a vdsm operation in a Cinder command? Line 63: return vdsReturnValue != null && vdsReturnValue.getSucceeded(); Line 64: } Line 65: Line 66: @Override Line 94: try { Line 95: Guid storagePoolId = getDiskImage().getStoragePoolId() != null ? getDiskImage().getStoragePoolId() Line 96: : Guid.Empty; Line 97: Guid storageDomainId = Line 98: getDiskImage().getStorageIds() != null && !getDiskImage().getStorageIds().isEmpty() ? getDiskImage().getStorageIds() please split the statement. it doesn't look good :) Line 99: .get(0) Line 100: : Guid.Empty; Line 101: Guid imageGroupId = getDiskImage().getimage_group_id() != null ? getDiskImage().getimage_group_id() Line 102: : Guid.Empty; Line 102: : Guid.Empty; Line 103: Line 104: Guid taskId = persistAsyncTaskPlaceHolder(VdcActionType.RestoreAllSnapshots); Line 105: Line 106: vdsReturnValue = runVdsCommand(VDSCommandType.DestroyImage, why do we invoke a vdsm command? Line 107: PostZeroHandler.fixParametersWithPostZero( Line 108: new DestroyImageVDSCommandParameters(storagePoolId, storageDomainId, imageGroupId, Line 109: _imagesToDelete, getDiskImage().isWipeAfterDelete(), true))); Line 110: Line 117: storageDomainId)); Line 118: } Line 119: } Line 120: // Don't throw an exception when cannot destroy image in the VDSM. Line 121: catch (VdcBLLException e) { move to EOL of line 119 Line 122: // Set fault for parent command RestoreAllSnapshotCommand to use, if decided to fail the command. Line 123: getReturnValue().setFault(new VdcFault(e, e.getVdsError().getCode())); Line 124: log.info("Image '{}' not exist in Irs", getDiskImage().getImageId()); Line 125: } -- To view, visit https://gerrit.ovirt.org/42337 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibd2c3ed4b2f2d0684476d9df9090d95dda72fd4e Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
