Darshan N has posted comments on this change. Change subject: gluster: Added geo-replication status and status detail ......................................................................
Patch Set 11: (6 comments) http://gerrit.ovirt.org/#/c/18414/11//COMMIT_MSG Commit Message: Line 11: *glusterGeoRepStatus- It provides geo-replication session status of all Line 12: sessions/between specified master and all its slaves/between a specified Line 13: master and a slave. Line 14: *glusterGeoRepStatusDetail- It provides detailed status of geo-replication Line 15: session between a master and slave. > Please provide more comment log including output dict of each verbs Done Line 16: Line 17: Change-Id: I4f37f35a5480fbe049a67758e122d4a0c2eba513 http://gerrit.ovirt.org/#/c/18414/11/client/vdsClientGluster.py File client/vdsClientGluster.py: Line 761: 'remoteVolumeName=<slave_volume_name> ' Line 762: '<master_volume_name>existing volume name in the master node\n\t' Line 763: '<slave_host_name>is remote slave host name or ip\n\t' Line 764: '<slave_volume_name>existing volume name in the slave node', Line 765: 'Returns ths status of geo-replication' > Please replace master/slave to local/remote Done Line 766: )), Line 767: 'glusterVolumeGeoRepStatusDetail': ( Line 768: serv.do_glusterVolumeGeoRepStatusDetail, Line 769: ('volumeName=<master_volume_name> ' Line 771: 'remoteVolumeName=<slave_volume_name> ' Line 772: '<master_volume_name>existing volume name in the master node\n\t' Line 773: '<slave_host_name>is remote slave host name or ip\n\t' Line 774: '<slave_volume_name>existing volume name in the slave node', Line 775: 'Returns the Detailed status of geo-replication' > same here Done Line 776: )), http://gerrit.ovirt.org/#/c/18414/11/vdsm/gluster/api.py File vdsm/gluster/api.py: Line 327: remoteVolumeName=None, options=None): Line 328: status = self.svdsmProxy.glusterVolumeGeoRepStatus(volumeName, Line 329: remoteHost, Line 330: remoteVolumeName) Line 331: return {'geo-rep': status} > I would suggest to have the key as geo-rep-status Done Line 332: Line 333: @exportAsVerb Line 334: def volumeGeoRepStatusDetail(self, volumeName, remoteHost, Line 335: remoteVolumeName, options=None): Line 337: volumeName, Line 338: remoteHost, Line 339: remoteVolumeName Line 340: ) Line 341: return {'geo-rep': status} > What is the difference with above verb? Can't we have detail=True arg to a Since the argument they take are different( in case of status volume name, remote host name and remote volume name are optional but in status detail all these arguments are mandatory) and even the return values are different, I feel having different verbs is better. Line 342: Line 343: Line 344: def getGlusterMethods(gluster): Line 345: l = [] http://gerrit.ovirt.org/#/c/18414/11/vdsm/gluster/cli.py File vdsm/gluster/cli.py: Line 1138: sessionDetail['pairs'] = pairs Line 1139: sessions.append(sessionDetail) Line 1140: volumeDetail['sessions'] = sessions Line 1141: status.append(volumeDetail) Line 1142: return status > 1. Please all replace master/slave to local/remote Since the names(master/slave) are obtained from glusterfs cli, would like to retain the names as is. Any thoughts ?? Line 1143: Line 1144: Line 1145: @makePublic Line 1146: def volumeGeoRepStatus(volumeName=None, remoteHost=None, -- To view, visit http://gerrit.ovirt.org/18414 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4f37f35a5480fbe049a67758e122d4a0c2eba513 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Darshan N <dnara...@redhat.com> Gerrit-Reviewer: Aravinda VK <avish...@redhat.com> Gerrit-Reviewer: Bala.FA <barum...@redhat.com> Gerrit-Reviewer: Better Saggi <bettersa...@gmail.com> Gerrit-Reviewer: Darshan N <dnara...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Timothy Asir <tjeya...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
