Daniel Erez has posted comments on this change. Change subject: core: avoid broken status on snapshot removal ......................................................................
Patch Set 6: (7 comments) http://gerrit.ovirt.org/#/c/27611/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java: Line 192: getSnapshotDao().remove(getParameters().getSnapshotId()); Line 193: } else { Line 194: List<String> failedToRemoveDisks = new ArrayList<>(); Line 195: Line 196: // Remove successfully deleted images from the snapshot > The comment for successfully deleted images from the snapshot should be at Done Line 197: for (VdcActionParametersBase parametersBase : getParameters().getImagesParameters()) { Line 198: ImagesContainterParametersBase imagesParams = (ImagesContainterParametersBase) parametersBase; Line 199: Line 200: if (imagesParams.getTaskGroupSuccess()) { Line 194: List<String> failedToRemoveDisks = new ArrayList<>(); Line 195: Line 196: // Remove successfully deleted images from the snapshot Line 197: for (VdcActionParametersBase parametersBase : getParameters().getImagesParameters()) { Line 198: ImagesContainterParametersBase imagesParams = (ImagesContainterParametersBase) parametersBase; > in case of snapshot with memory, seems that we might get here with other ty The memory parameters aren't added to the ImagesParameters (which probably should be fixed regardless...). For now, I'll change to safe casting. Line 199: Line 200: if (imagesParams.getTaskGroupSuccess()) { Line 201: Snapshot snapshotWithoutImage = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage( Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 197: for (VdcActionParametersBase parametersBase : getParameters().getImagesParameters()) { Line 198: ImagesContainterParametersBase imagesParams = (ImagesContainterParametersBase) parametersBase; Line 199: Line 200: if (imagesParams.getTaskGroupSuccess()) { Line 201: Snapshot snapshotWithoutImage = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage( > On each disk you are fetching the snapshot from the DB. I'll check if it doesn't make the code too complex.. Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 203: getSnapshotDao().update(snapshotWithoutImage); Line 204: } Line 205: else { Line 199: Line 200: if (imagesParams.getTaskGroupSuccess()) { Line 201: Snapshot snapshotWithoutImage = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage( Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 203: getSnapshotDao().update(snapshotWithoutImage); > suggestion - we can improve things a bit here and perform only one update a same Line 204: } Line 205: else { Line 206: log.errorFormat("Could not delete image {0} from snapshot {1}", Line 207: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 201: Snapshot snapshotWithoutImage = ImagesHandler.prepareSnapshotConfigWithoutImageSingleImage( Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 203: getSnapshotDao().update(snapshotWithoutImage); Line 204: } Line 205: else { > please use one liner for else, same in line 191 Done Line 206: log.errorFormat("Could not delete image {0} from snapshot {1}", Line 207: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 208: Line 209: DiskImage diskImage = getDiskImageDao().getSnapshotById(imagesParams.getImageId()); Line 202: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 203: getSnapshotDao().update(snapshotWithoutImage); Line 204: } Line 205: else { Line 206: log.errorFormat("Could not delete image {0} from snapshot {1}", > 1) you mismatched the parameters here. {0} should be the image id. Done Line 207: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 208: Line 209: DiskImage diskImage = getDiskImageDao().getSnapshotById(imagesParams.getImageId()); Line 210: failedToRemoveDisks.add(diskImage.getDiskAlias()); Line 206: log.errorFormat("Could not delete image {0} from snapshot {1}", Line 207: getParameters().getSnapshotId(), imagesParams.getImageId()); Line 208: Line 209: DiskImage diskImage = getDiskImageDao().getSnapshotById(imagesParams.getImageId()); Line 210: failedToRemoveDisks.add(diskImage.getDiskAlias()); > i'd add this also to the logging so it'll be easier to track.. "image 111 o Isn't the log in line 206 sufficient? Line 211: } Line 212: } Line 213: Line 214: if (!failedToRemoveDisks.isEmpty()) { -- To view, visit http://gerrit.ovirt.org/27611 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2a15034185ef30f53fbeaa46440cd92954d39eee Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
