Nir Soffer has posted comments on this change. Change subject: gluster: Set mount path based on gluster volume info ......................................................................
Patch Set 1: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/48308/1/vdsm/storage/storageServer.py File vdsm/storage/storageServer.py: Line 336 Line 337 Line 338 Line 339 Line 340 We know that this is the first server, so we should remove using pop(0), or use a slice: servers[1:] And of course we don't have any test for this change - we should start with a failing test. Line 335: if replicaCount not in self.ALLOWED_REPLICA_COUNTS: Line 336: raise se.UnsupportedGlusterVolumeReplicaCountError(replicaCount) Line 337: Line 338: def _get_backup_servers_option(self): Line 339: self._volfileserver = self.volinfo['bricks'][0].split(":")[0] This is messy. You start with value A, then you switch to value B. Please change the name of the instance variable to reflect the fact that this is the server we connect to get the gluster volume info, but the gluster volfileserver may be another server. Also you are spliting the bricks list twice, duplicating the code bellow. Line 340: servers = [brick.split(":")[0] for brick in self.volinfo['bricks']] Line 341: servers.remove(self._volfileserver) Line 342: if not servers: Line 343: return "" -- To view, visit https://gerrit.ovirt.org/48308 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id3386b37cd407c52e1b8f38d54c236bffc143e2f Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
