Update patch set 5
Patch Set 5: (7 comments) Hi Robert; I really struggled with this review and I think it's because it's retaining 'old' function names but doing something very different. I've had a go at re-writing the algorithm to a generic, recursive flatten to try to make things simpler, but this may not be what you were looking for? Patch-set: 5 Reviewer: Gerrit User 20870 <20870@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
This commit is contained in:
Gerrit User 20870
Gerrit Code Review
parent
02b0ec34e4
commit
6d34974bfd
|
|
@ -1,5 +1,51 @@
|
|||
{
|
||||
"comments": [
|
||||
{
|
||||
"key": {
|
||||
"uuid": "af4c92f9_90d9bd12",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 5
|
||||
},
|
||||
"lineNbr": 520,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "As `str` is immutable, they can be used as default values in the parameter list, which would negate the need to do `region or \"\"`. i.e. you don\u0027t need to use None.",
|
||||
"range": {
|
||||
"startLine": 509,
|
||||
"startChar": 0,
|
||||
"endLine": 520,
|
||||
"endChar": 30
|
||||
},
|
||||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "3c75a2fe_c629a5cd",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 5
|
||||
},
|
||||
"lineNbr": 578,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "\"naming things\" is one of the hard things in software development, and I think this is a misleading name, for the following reasons:\n\n * It doesn\u0027t load a tree. It actually loads a flat list of dictionaries.\n * It doesn\u0027t return a tree. It returns a map of id -\u003e the node of that id, which is half of a Schwartzian transform on the list of dicts by \u0027id\u0027.\n * It\u0027s actually doing two things; json loading the str, and then building a lookup-by-id dictionary.\n * It would be really helpful to actually say this.\n\nI don\u0027t actually think you need this function. There are two things going on here, loading json (which ought to by in a try: except: to capture errors for logging, etc.) and the transform which is purely (but a good thing) a performance enhancement for the algorithm. I would recommend splitting this into two functions or, better, just use them in the main function below.",
|
||||
"range": {
|
||||
"startLine": 577,
|
||||
"startChar": 0,
|
||||
"endLine": 578,
|
||||
"endChar": 47
|
||||
},
|
||||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "47c7b38e_834e872c",
|
||||
|
|
@ -17,6 +63,29 @@
|
|||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "76f03583_f5e0ed18",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 5
|
||||
},
|
||||
"lineNbr": 617,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "NO! Isn\u0027t this is a Dict[int, List[Dict[str, Union[str, int, List[int]]]]] ?\n\nAt least I think that it\u0027s either an int or a str (i.e. id or str or list of ids (children))\n\nThis is why this is so confusing.",
|
||||
"range": {
|
||||
"startLine": 617,
|
||||
"startChar": 18,
|
||||
"endLine": 617,
|
||||
"endChar": 38
|
||||
},
|
||||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "54b9c32a_7fd7eee3",
|
||||
|
|
@ -67,6 +136,75 @@
|
|||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "ae833a06_e7bf0cbc",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 5
|
||||
},
|
||||
"lineNbr": 653,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "It would be good to add a :rtype: of `List[CrushLocation]` to be clear on what it is returning.",
|
||||
"range": {
|
||||
"startLine": 653,
|
||||
"startChar": 14,
|
||||
"endLine": 653,
|
||||
"endChar": 19
|
||||
},
|
||||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "1a8fa696_b77162db",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 5
|
||||
},
|
||||
"lineNbr": 663,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "Because the json.loads() is now in a sub-function, the try: except: (although doing the right thing), is now disconnected from what it was originally capturing, which increases the cognitive load on the reader. e.g. \"What throws ValueError here? _load_ceph_osd_tree() or _get_childer() or CrushLoction()\"?\n\nAlso, \"nodes\" is not a good name for what that name is representing. it\u0027s a id_to_node_map or something similar.",
|
||||
"range": {
|
||||
"startLine": 663,
|
||||
"startChar": 12,
|
||||
"endLine": 663,
|
||||
"endChar": 45
|
||||
},
|
||||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "e2b53e73_893c13a4",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 5
|
||||
},
|
||||
"lineNbr": 666,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "Okay, I *think* I\u0027ve worked out what is going on here, but please correct me if I\u0027m wrong.\n\n * the output of ceph ... osd tree is a attribute-value list which represents multiple trees, with roots of type \"root\".\n * We\u0027ll assume that a tree can have leaf nodes that are in other trees.\n * The return value is a flattened list of host nodes, with attributes from the other linked nodes (via leaf nodes (children)).\n * This is essentially a de-normalised self joined table as a spreadsheet, but then indexed by the \"host\" node \"id\" as \"identifier\", and returned as [CrushLocation()]\n * Assumption is that the tree is a DAG (i.e. no cycles)\n\nIf this is true, then I would approach this problem in two steps:\n\n1. flatten the tree by root node into a list of dictionaries (special case for host to keeps its id as \u0027identifier\u0027.\n2. map the dictionaries into CrushLocation() list.\n\nFlatten is essentially:\n\n\n def flatten(node, node_lookup_map):\n attribute_dict \u003d {node[\u0027type\u0027]: node[\u0027name\u0027]}\n if node[\u0027type\u0027] \u003d\u003d \u0027host\u0027:\n attribute_dict[\u0027identifier\u0027] \u003d node[\u0027id\u0027]\n descendant_attribute_dicts \u003d [\n flatten(node_lookup_map[node_id])\n for node_id in node.get(\"children\", [])]\n if descendant_attribute_dicts:\n return [attribute.copy().update(descendant_attribute_dict)\n for descendant_attribute_dict in descendant_attribute_dicts]\n else:\n return [attribute] \n\n def flatten_roots(nodes):\n let lookup_map \u003d {node[\u0027id\u0027]: node for node in nodes}\n root_attributes_dicts \u003d [flatten(node, lookup_map)\n for node in nodes\n if node[\u0027type\u0027] \u003d\u003d \u0027root\u0027]\n # get a flattened list of roots.\n return itertools.chain(*root_attributes_dicts)\n\n # finally for the (misnamed?) get_osd_tree\n\n def get_osd_tree(service):\n # do the stuff to get the nodes.\n return [CrushLocation(**host) for host in flatten_roots(nodes)]\n\n\nIs this what it is supposed to be doing?",
|
||||
"range": {
|
||||
"startLine": 664,
|
||||
"startChar": 0,
|
||||
"endLine": 666,
|
||||
"endChar": 79
|
||||
},
|
||||
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
}
|
||||
]
|
||||
}
|
||||
|
|
@ -1,5 +1,28 @@
|
|||
{
|
||||
"comments": [
|
||||
{
|
||||
"key": {
|
||||
"uuid": "9a1d8040_b3d4daeb",
|
||||
"filename": "charms_ceph/utils.py",
|
||||
"patchSetId": 4
|
||||
},
|
||||
"lineNbr": 506,
|
||||
"author": {
|
||||
"id": 20870
|
||||
},
|
||||
"writtenOn": "2021-06-27T15:56:47Z",
|
||||
"side": 1,
|
||||
"message": "Strings aren\u0027t mutable, so it\u0027s okay to use them as a default in the functions argument list. i.e. you don\u0027t need to to `self.osd \u003d osd or \"\"`",
|
||||
"range": {
|
||||
"startLine": 504,
|
||||
"startChar": 0,
|
||||
"endLine": 506,
|
||||
"endChar": 69
|
||||
},
|
||||
"revId": "9b34af1aea048f72cb1bd840aec19a5068a83009",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": true
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "34b08069_38a4e1fb",
|
||||
|
|
|
|||
Loading…
Reference in New Issue