Shubhendu Tripathi 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? Actually not required as we dont need the persisted states in further calls. Will remove this. Line 78: handleVdsError > restore or deleteall failed? Will correct this. Copy paste issue :) Line 127: > if slave volume is not maintained by engine, are we preventing snapshot del May be we can allow this and warning message before delete action can tell that make sure slave snapshots are deleted if georep enabled and slave cluster not maintained by engine. What you say?? 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? Not required. will remove this Line 71: > error is incorrect done 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 May be we can allow with proper warning in confirmation dialog 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 - other thats correct. will implement this. didnt realize that internal action would not acquire lock 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 s done -- 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
