Ala Hino has posted comments on this change. Change subject: gluster: Remove duplicate servers ......................................................................
Patch Set 5: (6 comments) https://gerrit.ovirt.org/#/c/49023/5/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1257: if pid_file is not None: Line 1258: rmFile(pid_file) Line 1259: Line 1260: Line 1261: def unique(collection): > Please rename to iterable - this is not a collection but something you can Done Line 1262: """ Line 1263: Remove duplicates from provided collection Line 1264: """ Line 1265: if not collection: Line 1261: unique Renamed to unique_list because it gets an iterable object (list, tuple, generator, etc) and returns a list Line 1259: Line 1260: Line 1261: def unique(collection): Line 1262: """ Line 1263: Remove duplicates from provided collection > Please check my comment in previous versions about hashable objects. Don't understand the comment, please elaborate Line 1264: """ Line 1265: if not collection: Line 1266: return collection Line 1262: """ Line 1263: Remove duplicates from provided collection Line 1264: """ Line 1265: if not collection: Line 1266: return collection > We don't need this shortcut, and it may break for a collection implementing Done https://gerrit.ovirt.org/#/c/49023/5/tests/utilsTests.py File tests/utilsTests.py: Line 282: ((i for i in [1, 2, 3, 1, 3]), [1, 2, 3]), Line 283: (('a', 'a', 'b', 'c', 'a', 'd'), ['a', 'b', 'c', 'd']), Line 284: (['a', 'a', 'b', 'c', 'a', 'd'], ['a', 'b', 'c', 'd']) Line 285: ]) Line 286: def testUnique(self, collection, uniqueCollection): > Please use only lowercase_names. Also the input is should be called iterabl Done Line 287: self.assertEquals(utils.unique(collection), uniqueCollection) Line 288: Line 289: Line 290: class AsyncProcessOperationTests(TestCaseBase): https://gerrit.ovirt.org/#/c/49023/5/vdsm/storage/storageServer.py File vdsm/storage/storageServer.py: Line 338: raise se.UnsupportedGlusterVolumeReplicaCountError(replicaCount) Line 339: Line 340: def _get_backup_servers_option(self): Line 341: servers = utils.unique([brick.split(":")[0] for brick Line 342: in self.volinfo['bricks']]) > Please remove the temporary list, unique can handle generators: Done Line 343: self.log.debug("Using bricks: %s", servers) Line 344: if self._volfileserver in servers: Line 345: servers.remove(self._volfileserver) Line 346: else: -- To view, visit https://gerrit.ovirt.org/49023 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10fd0c985e7fe024460faa288fc790e761da4811 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
