Sahina Bose has posted comments on this change. Change subject: gluster: Gluster volume snapshot actions with georep case ......................................................................
Patch Set 6: (8 comments) https://gerrit.ovirt.org/#/c/38150/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteAllGlusterVolumeSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteAllGlusterVolumeSnapshotsCommand.java: Line 66: why in a separate transaction? Line 78: handleVdsError restore or deleteall failed? Line 127: if slave volume is not maintained by engine, are we preventing snapshot delete? What if user has already done the required slave actions? https://gerrit.ovirt.org/#/c/38150/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteGlusterVolumeSnapshotCommand.java: Line 59: is separate transaction required? Line 71: error is incorrect Line 100: } Line 101: Line 102: for (GlusterGeoRepSession session : georepSessions) { Line 103: if (session.getSlaveVolumeId() == null || session.getSlaveNodeUuid() == null) { Line 104: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_REMOTE_CLUSTER_NOT_MAINTAINED_BY_ENGINE); same comment as previous command Line 105: } Line 106: } Line 107: Line 108: return true; https://gerrit.ovirt.org/#/c/38150/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/RestoreGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/RestoreGlusterVolumeSnapshotCommand.java: Line 59: Don't you need to acquire lock on cluster as you're stopping volume - otherwise sync job may update this too. Same comment for other geo rep actions..lock on georepsession? The runInternalAction would not acquire a lock. So getExclusiveLocks of this command may need to be overridden? https://gerrit.ovirt.org/#/c/38150/6/backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties: Line 299: NoUpServerFoundInSlaveCluster Make this message generic, so you can reuse in other commands like Delete snapshot in slave cluster? -- To view, visit https://gerrit.ovirt.org/38150 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0eccbcd4aa3e218ba0d910bd9150bdb0baa2db68 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.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
