Ido Barkan has posted comments on this change. Change subject: storage: introducing vdsm-dump-chains script ......................................................................
Patch Set 7: (14 comments) https://gerrit.ovirt.org/#/c/38281/7/client/vdsm_dump_chains.py File client/vdsm_dump_chains.py: Line 19: Line 20: import socket Line 21: import sys Line 22: import optparse Line 23: import errno > Should be sorted Done Line 24: from vdsm import vdscli Line 25: # BLANK_UUID is re-declared here since it cannot be imported properly. this Line 26: # constant should be introduced under lib publically available Line 27: BLANK_UUID = '00000000-0000-0000-0000-000000000000' Line 20: import socket Line 21: import sys Line 22: import optparse Line 23: import errno Line 24: from vdsm import vdscli > Add empty line Done Line 25: # BLANK_UUID is re-declared here since it cannot be imported properly. this Line 26: # constant should be introduced under lib publically available Line 27: BLANK_UUID = '00000000-0000-0000-0000-000000000000' Line 28: Line 23: import errno Line 24: from vdsm import vdscli Line 25: # BLANK_UUID is re-declared here since it cannot be imported properly. this Line 26: # constant should be introduced under lib publically available Line 27: BLANK_UUID = '00000000-0000-0000-0000-000000000000' > If _USAGE is private, so is BLANK_UUID. Either make both private or both pu Done Line 28: Line 29: _USAGE = "usage: vdsm-dump-chains [options] <sd_UUID>" Line 30: Line 31: Line 30: Line 31: Line 32: class ServerError(Exception): Line 33: def __init__(self, server_result): Line 34: super(ServerError, self).__init__() > Not needed Done Line 35: self.err_code = server_result['status']['code'] Line 36: self.msg = server_result['status']['message'] Line 37: Line 38: def __str__(self): Line 32: class ServerError(Exception): Line 33: def __init__(self, server_result): Line 34: super(ServerError, self).__init__() Line 35: self.err_code = server_result['status']['code'] Line 36: self.msg = server_result['status']['message'] > I would be nicer to use the same terms used in the server status: Done Line 37: Line 38: def __str__(self): Line 39: return 'server error. code: %s msg: %s' % (self.err_code, self.msg) Line 40: Line 35: self.err_code = server_result['status']['code'] Line 36: self.msg = server_result['status']['message'] Line 37: Line 38: def __str__(self): Line 39: return 'server error. code: %s msg: %s' % (self.err_code, self.msg) > Same, use "code" and "message" Done Line 40: Line 41: Line 42: class ChainError(Exception): Line 43: def __init__(self, volumes_children): Line 40: Line 41: Line 42: class ChainError(Exception): Line 43: def __init__(self, volumes_children): Line 44: super(ChainError, self).__init__() > Not needed Done Line 45: self.volumes_children = volumes_children Line 46: Line 47: def __str__(self): Line 48: return 'volumes and parents: ' + ' '.join( Line 41: Line 42: class ChainError(Exception): Line 43: def __init__(self, volumes_children): Line 44: super(ChainError, self).__init__() Line 45: self.volumes_children = volumes_children > I wonder if we should call this "relations" or some other name related to p Lets stick with the current name, as it describes the data structure itself Line 46: Line 47: def __str__(self): Line 48: return 'volumes and parents: ' + ' '.join( Line 49: ['%s<-%s' % (p, c) for p, c in self.volumes_children]) Line 45: self.volumes_children = volumes_children Line 46: Line 47: def __str__(self): Line 48: return 'volumes and parents: ' + ' '.join( Line 49: ['%s<-%s' % (p, c) for p, c in self.volumes_children]) > Lets use "parent" and "child" to make this easier to parse for humans. Done Line 50: Line 51: Line 52: class DuplicateParentError(ChainError): Line 53: pass Line 49: ['%s<-%s' % (p, c) for p, c in self.volumes_children]) Line 50: Line 51: Line 52: class DuplicateParentError(ChainError): Line 53: pass > It would be nicer to replace "pass" with a docstring explaining the nature Done Line 54: Line 55: Line 56: class NoBaseVolume(ChainError): Line 57: pass https://gerrit.ovirt.org/#/c/38281/7/tests/vdsmDumpChainsTests.py File tests/vdsmDumpChainsTests.py: Line 18: # Line 19: Line 20: from testlib import VdsmTestCase as TestCaseBase Line 21: from vdsm_dump_chains import _get_volume_chain, ChainError, BLANK_UUID, \ Line 22: OrphanVolumes, ChainLoopError, NoBaseVolume, DuplicateParentError > Lets use import (name, ...) Done Line 23: Line 24: Line 25: class VdsmDumpChainsTest(TestCaseBase): Line 26: def test_empty(self): Line 21: from vdsm_dump_chains import _get_volume_chain, ChainError, BLANK_UUID, \ Line 22: OrphanVolumes, ChainLoopError, NoBaseVolume, DuplicateParentError Line 23: Line 24: Line 25: class VdsmDumpChainsTest(TestCaseBase): > This tests only _get_volume_chain, so lets call it GetVolumeChainTests Done Line 26: def test_empty(self): Line 27: self.assertEqual(_get_volume_chain([]), []) Line 28: Line 29: def test_only_base_volume(self): Line 29: def test_only_base_volume(self): Line 30: self.assertEqual(_get_volume_chain([(BLANK_UUID, 'a')]), ['a']) Line 31: Line 32: def test_orphan_volumes(self): Line 33: volumes = [(BLANK_UUID, 'a'), ('a', 'b'), ('c', 'd')] > Lets use the same term you use for this data in the code: "volumes_children Done Line 34: with self.assertRaises(OrphanVolumes) as cm: Line 35: _get_volume_chain(volumes) Line 36: self.assertEqual(cm.exception.volumes_children, volumes) Line 37: Line 36: self.assertEqual(cm.exception.volumes_children, volumes) Line 37: Line 38: def test_simple_chain(self): Line 39: self.assertEqual(_get_volume_chain( Line 40: [(BLANK_UUID, 'a'), ('a', 'b'), ('b', 'c')]), ['a', 'b', 'c']) > Use a temporary to make this test more clear: Done Line 41: Line 42: def test_loop(self): Line 43: with self.assertRaises(ChainLoopError): Line 44: _get_volume_chain([ -- To view, visit https://gerrit.ovirt.org/38281 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I428c443bb7d6b2a504a6f77efcd4838f7ae6c404 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ido Barkan <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Darshan N <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vladik Romanovsky <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yaniv Dary <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
