From 6d34974bfd7a0d4837ae74d0a1453edaadd6def3 Mon Sep 17 00:00:00 2001 From: Gerrit User 20870 <20870@4a232e18-c5a9-48ee-94c0-e04e7cca6543> Date: Sun, 27 Jun 2021 15:56:47 +0000 Subject: [PATCH] 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> --- 16fa10433ca040b01f6216b9ef95ec97de54eb0e | 138 +++++++++++++++++++++++ 9b34af1aea048f72cb1bd840aec19a5068a83009 | 23 ++++ 2 files changed, 161 insertions(+) diff --git a/16fa10433ca040b01f6216b9ef95ec97de54eb0e b/16fa10433ca040b01f6216b9ef95ec97de54eb0e index 8e08c2c..3a737f8 100644 --- a/16fa10433ca040b01f6216b9ef95ec97de54eb0e +++ b/16fa10433ca040b01f6216b9ef95ec97de54eb0e @@ -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 } ] } \ No newline at end of file diff --git a/9b34af1aea048f72cb1bd840aec19a5068a83009 b/9b34af1aea048f72cb1bd840aec19a5068a83009 index 67559a5..321851d 100644 --- a/9b34af1aea048f72cb1bd840aec19a5068a83009 +++ b/9b34af1aea048f72cb1bd840aec19a5068a83009 @@ -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",