openstack
/
charms.ceph
Code Issues Proposed changes

Update patch set 6 

Patch Set 6:

(10 comments)

Thank you both for the review.

Alex: I used your suggestion and tried to taste your functions as well, but without success. Do you want me to look at it better? FYI add a link to pastebin with the python script on which I tested it.

Aurelien: I also added your suggestions and at the same time changed the recursion to the second pattern. Thank you for a very suitable comment, really mixing these two patterns caused complications when reading.

Patch-set: 6
This commit is contained in:
Gerrit User 32363 2021-06-28 17:26:54 +00:00 committed by Gerrit Code Review
parent 36ae7788a1
commit 7eeba72258
2 changed files with 216 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

192
16fa10433ca040b01f6216b9ef95ec97de54eb0e
View File

@ -23,6 +23,30 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "7125e3bd_63e9daab",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 520,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "Done",
"parentUuid": "af4c92f9_90d9bd12",
"range": {
"startLine": 509,
"startChar": 0,
"endLine": 520,
"endChar": 30
},
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "3c75a2fe_c629a5cd",
@ -46,6 +70,30 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "85491094_764e0e47",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 578,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "Ack",
"parentUuid": "3c75a2fe_c629a5cd",
"range": {
"startLine": 577,
"startChar": 0,
"endLine": 578,
"endChar": 47
},
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "47c7b38e_834e872c",
@ -63,6 +111,24 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "eb95dac2_d6326910",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 589,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "You\u0027re right, it should. Actually it\u0027s returning only grand-(grand-(...))-children and stops at the desired type or finds the youngest descendant (lowest in CRUSH hierarchy).",
"parentUuid": "47c7b38e_834e872c",
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "76f03583_f5e0ed18",
@ -86,6 +152,30 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "5924e21c_60aa4e0f",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 617,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "You\u0027re right 💯, this really good example of why the `_load_ceph_osd_tree` function is just confusing. When I wrote this docstring, I only looked at json.",
"parentUuid": "76f03583_f5e0ed18",
"range": {
"startLine": 617,
"startChar": 18,
"endLine": 617,
"endChar": 38
},
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "54b9c32a_7fd7eee3",
@ -103,6 +193,24 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "e365f8e0_0f2e1c68",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 625,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "Done",
"parentUuid": "54b9c32a_7fd7eee3",
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "8c22b177_2e6cffcb",
@ -120,6 +228,24 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "1da1c6db_8b039332",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 626,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "Done",
"parentUuid": "8c22b177_2e6cffcb",
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "9f2da9d5_82c3d518",
@ -137,6 +263,24 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "e67d3e12_e4998c4c",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 645,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "Yes, you are right. I mixed both patterns and I will use the second one as you suggested, because I don\u0027t see any simple solution how the aggregate returns from recursion.",
"parentUuid": "9f2da9d5_82c3d518",
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "ae833a06_e7bf0cbc",
@ -160,6 +304,30 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "e292d516_022e5f1a",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 653,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "Done",
"parentUuid": "ae833a06_e7bf0cbc",
"range": {
"startLine": 653,
"startChar": 14,
"endLine": 653,
"endChar": 19
},
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "1a8fa696_b77162db",
@ -205,6 +373,30 @@
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "cf6a58c7_b2156aab",
"filename": "charms_ceph/utils.py",
"patchSetId": 5
},
"lineNbr": 666,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "* Yes, that\u0027s correct\n* No, a leaf can only be on one tree\n* Yes, that\u0027s correct\n* Yes, that\u0027s correct (I think)\n* Yes, that\u0027s correct\n\nI was not able to tested [1] it your suggestion, because `descendant_attribute_dicts` could be [], [[{\u0027osd\u0027: \u0027osd.1\u0027}]] or [[None]].\n\n---\n[1]: https://pastebin.canonical.com/p/GjpT42BQg3/",
"parentUuid": "e2b53e73_893c13a4",
"range": {
"startLine": 664,
"startChar": 0,
"endLine": 666,
"endChar": 79
},
"revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
}
]
}

24
9b34af1aea048f72cb1bd840aec19a5068a83009
View File

@ -23,6 +23,30 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "a9877583_fe1c526d",
"filename": "charms_ceph/utils.py",
"patchSetId": 4
},
"lineNbr": 506,
"author": {
"id": 32363
},
"writtenOn": "2021-06-28T17:26:54Z",
"side": 1,
"message": "You are right, string aren\u0027t mutable. However, I think this way it will handle a better situation when someone pass a parameter like None. e.g CrushLocation(..., osd\u003dNone)\n\nThe fair point is that it is not used in any other function, only in get_osd_tree. [1] \u003d\u003e It can be changed without any harm and I will change it.\n---\n[1]: https://codesearch.opendev.org/?q\u003dCrushLocation\u0026i\u003dnope\u0026files\u003d\u0026excludeFiles\u003d\u0026repos\u003d",
"parentUuid": "9a1d8040_b3d4daeb",
"range": {
"startLine": 504,
"startChar": 0,
"endLine": 506,
"endChar": 69
},
"revId": "9b34af1aea048f72cb1bd840aec19a5068a83009",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": true
},
{
"key": {
"uuid": "34b08069_38a4e1fb",