Shubhendu Tripathi has posted comments on this change.

Change subject: gluster: BLL command for volume snapshot create
......................................................................


Patch Set 16:

(5 comments)

http://gerrit.ovirt.org/#/c/34928/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeSnapshotCommand.java
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeSnapshotCommand.java:

Line 67: 
Line 68:         // Get slave geo-rep sessions
Line 69:         if (geoRepSessions != null && geoRepSessions.size() > 0) {
Line 70:             for (GlusterGeoRepSession session : geoRepSessions) {
Line 71:                 if (session.getStatus() == GeoRepSessionStatus.ACTIVE) 
{
> Even faulty sessions may need to be paused?
I feel we can have a check as below

if (session.getStatus() != GeoRepSessionStatus.PAUSED)
Line 72:                     GlusterVolumeEntity slaveVolume =
Line 73:                             
getDbFacade().getGlusterVolumeDao().getById(session.getSlaveVolumeId());
Line 74:                     VDS slaveUpServer = 
ClusterUtils.getInstance().getRandomUpServer(slaveVolume.getClusterId());
Line 75: 


Line 69:         if (geoRepSessions != null && geoRepSessions.size() > 0) {
Line 70:             for (GlusterGeoRepSession session : geoRepSessions) {
Line 71:                 if (session.getStatus() == GeoRepSessionStatus.ACTIVE) 
{
Line 72:                     GlusterVolumeEntity slaveVolume =
Line 73:                             
getDbFacade().getGlusterVolumeDao().getById(session.getSlaveVolumeId());
> what if slaveVolumeId is null?
In that scenario I feel we should just continue and pause other sessions and 
create snapshots for them.
Is there any other way to pause session and create  snapshot?? I dont think so.
Line 74:                     VDS slaveUpServer = 
ClusterUtils.getInstance().getRandomUpServer(slaveVolume.getClusterId());
Line 75: 
Line 76:                     // Pause the geo-rep session
Line 77:                     VdcReturnValueBase sessionPauseRetVal =


Line 70:             for (GlusterGeoRepSession session : geoRepSessions) {
Line 71:                 if (session.getStatus() == GeoRepSessionStatus.ACTIVE) 
{
Line 72:                     GlusterVolumeEntity slaveVolume =
Line 73:                             
getDbFacade().getGlusterVolumeDao().getById(session.getSlaveVolumeId());
Line 74:                     VDS slaveUpServer = 
ClusterUtils.getInstance().getRandomUpServer(slaveVolume.getClusterId());
> up server should be checked on master volume
Yes. Following patch has the corrections.
Line 75: 
Line 76:                     // Pause the geo-rep session
Line 77:                     VdcReturnValueBase sessionPauseRetVal =
Line 78:                             
getBackend().runAction(VdcActionType.PauseGlusterVolumeGeoRepSession,


Line 75: 
Line 76:                     // Pause the geo-rep session
Line 77:                     VdcReturnValueBase sessionPauseRetVal =
Line 78:                             
getBackend().runAction(VdcActionType.PauseGlusterVolumeGeoRepSession,
Line 79:                                     new 
GlusterVolumeGeoRepSessionParameters(slaveVolume.getId(),
> to pause session, the master volume id should be passed as first parameter
Yes. Following patch has the corrections.
Line 80:                                             
session.getSlaveVolumeName(),
Line 81:                                             
session.getSlaveHostName()));
Line 82:                     if (!sessionPauseRetVal.getSucceeded()) {
Line 83:                         
handleVdsErrors(AuditLogType.GLUSTER_VOLUME_GEO_REP_PAUSE_FAILED,


Line 87:                     } else {
Line 88:                         // Create snapshot for slave volume
Line 89:                         VDSReturnValue snapCreationRetVal =
Line 90:                                 
runVdsCommand(VDSCommandType.CreateGlusterVolumeSnapshot,
Line 91:                                         new 
GlusterVolumeCreateSnapshotVDSParameters(slaveUpServer.getId(),
> here...you can only do this if slave is managed by oVirt. You may need to r
Suggestion of throwing an error to user and asking for confirmation to continue 
without geo-replicated volume's snapshots is good. Nice to give a try.
Currently canDoAction() fails if any of the geo-replicated volume's cluster is 
not maintained by engine.
Line 92:                                                 
session.getSlaveVolumeName(),
Line 93:                                                 
snapshot.getSnapshotName(),
Line 94:                                                 
snapshot.getDescription(),
Line 95:                                                 force));


-- 
To view, visit http://gerrit.ovirt.org/34928
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c27f9e73bac8d6de3022b0dd4e70aa0d8b1494b
Gerrit-PatchSet: 16
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: Ramesh N <rnach...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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

Reply via email to