)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"407d2661486e89680a70bd2c97cf734770f77d19","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"63da3abc_9438845a","updated":"2022-11-21 19:23:19.000000000","message":"So, things that bug me about this approach:\n\n- load() is a complete mess -- we\u0027ve basically scrapped all attempts at making a versioned file format from v1 and done yet another completely incompatible thing. https://xkcd.com/927/\n- New format gets a new name -- which seems appropriate given how it\u0027s no longer a gzip stream, but it (1) makes the auto-reloading tricky, especially when trying to auto-reload through the transition and (2) will require more significant changes to any ring-management tooling operators are using (such as our controller).\n\nThat said, I do like the accessibility via standard tooling -- I have high confidence that I could build a ring without ever needing to touch swift code if I wanted.","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"98ea373016c61b910e438651c914ce60d1b787cb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d12e2730_19a90ab6","updated":"2022-11-21 15:57:34.000000000","message":"this looks pretty snappy!","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"622c530cc70e4d54384bfdbd1b029bb628dd5b9c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"0cec5331_c93ea7b9","updated":"2022-12-01 15:58:15.000000000","message":"I have to admit i like this slightly better than having to write our own unzip cli https://review.opendev.org/c/openstack/swift/+/866082\n\nI\u0027m curious what other blockers and concerns you have for this approach:  upgrades? \n zipfile overhead/speed/acceleration?  cross-platform-support?","commit_id":"2121a3cae7faea5c8e71d40c4e2caded6b6bad4c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b49ee093e2961dcd731e02f6c5022d0470e2332","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"8b2a132f_12ade86e","in_reply_to":"0cec5331_c93ea7b9","updated":"2023-01-05 19:32:12.000000000","message":"IDK that I\u0027m opposed to having our own zip-like util -- if you\u0027re interacting with rings at this level, it seems likely that you\u0027re going to need swift installed anyway.\n\nUpgrades and automatic reloads are one set of worries -- if we have both a .ring.gz and a .ring.zip around, which gets loaded? Should it always prefer the .zip, or should we compare mtimes? If one of the two is invalid, should we try to load the other? Maybe only if the mtime indicates we\u0027re not going back in time?\n\nThe other is the required updates to operator tooling -- with the other chain, ops get an extra CLI flag, but nothing else should need to change. With this one, they need to know which which filename they\u0027re generating to make sure their (updated) tooling can copy out the proper ring, and clean up the old format once nodes get the new one. Then do all that in reverse if they discover they need to roll back.","commit_id":"2121a3cae7faea5c8e71d40c4e2caded6b6bad4c"}],"swift/common/ring/ring.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"98ea373016c61b910e438651c914ce60d1b787cb","unresolved":true,"context_lines":[{"line_number":343,"context_line":"        if next_part_power is not None:"},{"line_number":344,"context_line":"            _text[\u0027next_part_power\u0027] \u003d next_part_power"},{"line_number":345,"context_line":""},{"line_number":346,"context_line":"        with zipfile.ZipFile(filename, mode\u003d\u0027w\u0027) as zf:"},{"line_number":347,"context_line":"            info \u003d zipfile.ZipInfo(\"swift/ring/metadata\", gmtime(mtime)[:6])"},{"line_number":348,"context_line":"            zf.writestr(info, json.dumps(_text), zipfile.ZIP_DEFLATED)"},{"line_number":349,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7317798d_eb1ac54c","line":346,"updated":"2022-11-21 15:57:34.000000000","message":"I feel like we should set compression to ZIP_DEFLATED or something *here*\n\nhttps://docs.python.org/3/library/zipfile.html#zipfile.ZipFile","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"407d2661486e89680a70bd2c97cf734770f77d19","unresolved":true,"context_lines":[{"line_number":343,"context_line":"        if next_part_power is not None:"},{"line_number":344,"context_line":"            _text[\u0027next_part_power\u0027] \u003d next_part_power"},{"line_number":345,"context_line":""},{"line_number":346,"context_line":"        with zipfile.ZipFile(filename, mode\u003d\u0027w\u0027) as zf:"},{"line_number":347,"context_line":"            info \u003d zipfile.ZipInfo(\"swift/ring/metadata\", gmtime(mtime)[:6])"},{"line_number":348,"context_line":"            zf.writestr(info, json.dumps(_text), zipfile.ZIP_DEFLATED)"},{"line_number":349,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"f36ce481_deddd53d","line":346,"in_reply_to":"7317798d_eb1ac54c","updated":"2022-11-21 19:23:19.000000000","message":"I mean, we can, but it won\u0027t help. They even call it out in docs: https://docs.python.org/3/library/zipfile.html#zipfile.ZipFile.writestr\n\n\u003e When passing a ZipInfo instance as the zinfo_or_arcname parameter, the compression method used will be that specified in the compress_type member of the given ZipInfo instance. By default, the ZipInfo constructor sets this member to ZIP_STORED.","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"98ea373016c61b910e438651c914ce60d1b787cb","unresolved":true,"context_lines":[{"line_number":345,"context_line":""},{"line_number":346,"context_line":"        with zipfile.ZipFile(filename, mode\u003d\u0027w\u0027) as zf:"},{"line_number":347,"context_line":"            info \u003d zipfile.ZipInfo(\"swift/ring/metadata\", gmtime(mtime)[:6])"},{"line_number":348,"context_line":"            zf.writestr(info, json.dumps(_text), zipfile.ZIP_DEFLATED)"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"            info \u003d zipfile.ZipInfo(\"swift/ring/devices\", gmtime(mtime)[:6])"},{"line_number":351,"context_line":"            zf.writestr(info, json.dumps(ring[\u0027devs\u0027]), zipfile.ZIP_DEFLATED)"}],"source_content_type":"text/x-python","patch_set":2,"id":"8026b424_1900a870","line":348,"updated":"2022-11-21 15:57:34.000000000","message":"oic, so this is compress_type, not compresslevel - still maybe easier to set once on the constructor; unless later we\u0027re changing the compression type/level per object?","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"98ea373016c61b910e438651c914ce60d1b787cb","unresolved":true,"context_lines":[{"line_number":350,"context_line":"            info \u003d zipfile.ZipInfo(\"swift/ring/devices\", gmtime(mtime)[:6])"},{"line_number":351,"context_line":"            zf.writestr(info, json.dumps(ring[\u0027devs\u0027]), zipfile.ZIP_DEFLATED)"},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"            info \u003d zipfile.ZipInfo(\"swift/ring/assignments\", gmtime(mtime)[:6])"},{"line_number":354,"context_line":"            zf.writestr("},{"line_number":355,"context_line":"                info,"},{"line_number":356,"context_line":"                ring[\u0027replica2part2dev_id\u0027].to_network_order_bytes(),"}],"source_content_type":"text/x-python","patch_set":2,"id":"7dd800d3_4f2005d3","line":353,"updated":"2022-11-21 15:57:34.000000000","message":"you could use a variable and just convert the mtime to time tuple once","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"407d2661486e89680a70bd2c97cf734770f77d19","unresolved":true,"context_lines":[{"line_number":368,"context_line":"        tempf \u003d NamedTemporaryFile(dir\u003d\".\", prefix\u003dfilename, delete\u003dFalse)"},{"line_number":369,"context_line":"        if format_version \u003d\u003d 2:"},{"line_number":370,"context_line":"            tempf.close()  # ZipFile\u0027s just going to overwrite it anyway"},{"line_number":371,"context_line":"            self.serialize_v2(tempf.name, mtime)"},{"line_number":372,"context_line":"            # ZipFile will take care of flushing, but not fsyncing :-/"},{"line_number":373,"context_line":"        else:"},{"line_number":374,"context_line":"            # Override the timestamp so that the same ring data creates"}],"source_content_type":"text/x-python","patch_set":2,"id":"bdd2dbfe_4d95ed31","line":371,"range":{"start_line":371,"start_character":30,"end_line":371,"end_character":40},"updated":"2022-11-21 19:23:19.000000000","message":"I should read those docs more carefully -- ZipFile accepts either a string filename *or* a file-like object. I can just pass tempf here.","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"622c530cc70e4d54384bfdbd1b029bb628dd5b9c","unresolved":true,"context_lines":[{"line_number":284,"context_line":"                else:"},{"line_number":285,"context_line":"                    # Assume old-style pickled ring"},{"line_number":286,"context_line":"                    gz_file.seek(0)"},{"line_number":287,"context_line":"                    ring_data \u003d pickle.load(gz_file)"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":"        if not hasattr(ring_data, \u0027devs\u0027):"},{"line_number":290,"context_line":"            ring_data \u003d RingData(ring_data[\u0027replica2part2dev_id\u0027],"}],"source_content_type":"text/x-python","patch_set":5,"id":"4096c438_ebeb5960","line":287,"updated":"2022-12-01 15:58:15.000000000","message":"what if instead of opening/reading/peaking at the first few bytes ... we just look at the RingName: https://review.opendev.org/c/openstack/swift/+/866293\n\n... what are the chances some ops would be dumb enough to rename a v2 ring.zip to ring.gz just so they can file bugs!?","commit_id":"2121a3cae7faea5c8e71d40c4e2caded6b6bad4c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b49ee093e2961dcd731e02f6c5022d0470e2332","unresolved":true,"context_lines":[{"line_number":284,"context_line":"                else:"},{"line_number":285,"context_line":"                    # Assume old-style pickled ring"},{"line_number":286,"context_line":"                    gz_file.seek(0)"},{"line_number":287,"context_line":"                    ring_data \u003d pickle.load(gz_file)"},{"line_number":288,"context_line":""},{"line_number":289,"context_line":"        if not hasattr(ring_data, \u0027devs\u0027):"},{"line_number":290,"context_line":"            ring_data \u003d RingData(ring_data[\u0027replica2part2dev_id\u0027],"}],"source_content_type":"text/x-python","patch_set":5,"id":"fb479715_fa3e4b37","line":287,"in_reply_to":"4096c438_ebeb5960","updated":"2023-01-05 19:32:12.000000000","message":"*shrug* I\u0027m not sure it makes that much of a difference -- this function\u0027s still going to be a mess of special cases with no coherent story. At least with the custom format, we can use the format_version we already created.","commit_id":"2121a3cae7faea5c8e71d40c4e2caded6b6bad4c"}],"swift/common/ring/utils.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"98ea373016c61b910e438651c914ce60d1b787cb","unresolved":true,"context_lines":[{"line_number":120,"context_line":"            if six.PY2:"},{"line_number":121,"context_line":"                return self.array.tostring()"},{"line_number":122,"context_line":"            else:"},{"line_number":123,"context_line":"                return self.array.tobytes()"},{"line_number":124,"context_line":"        finally:"},{"line_number":125,"context_line":"            if sys.byteorder \u003d\u003d \u0027little\u0027:"},{"line_number":126,"context_line":"                self.array.byteswap()"}],"source_content_type":"text/x-python","patch_set":2,"id":"dedbc7ce_be6524c5","line":123,"updated":"2022-11-21 15:57:34.000000000","message":"is this a copy of the whole array - or some sort of memory view object?","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"407d2661486e89680a70bd2c97cf734770f77d19","unresolved":true,"context_lines":[{"line_number":120,"context_line":"            if six.PY2:"},{"line_number":121,"context_line":"                return self.array.tostring()"},{"line_number":122,"context_line":"            else:"},{"line_number":123,"context_line":"                return self.array.tobytes()"},{"line_number":124,"context_line":"        finally:"},{"line_number":125,"context_line":"            if sys.byteorder \u003d\u003d \u0027little\u0027:"},{"line_number":126,"context_line":"                self.array.byteswap()"}],"source_content_type":"text/x-python","patch_set":2,"id":"5bf7dc97_2239d75b","line":123,"in_reply_to":"dedbc7ce_be6524c5","updated":"2022-11-21 19:23:19.000000000","message":"It\u0027s calling tobytes() on the underlying large array -- so there\u0027s one copy of array -\u003e bytes before handing off to zipfile to do its thing, rather than N array -\u003e bytes copies for each replica, plus an additional copy to join them all together.\n\nMaybe if we basically rewrote ZipFile.writestr() ourselves we\u0027d have a way to use array.tofile() and avoid even that copy? (Or at any rate, punt it to stdlib to manage.) But the existing APIs feel really limited: https://github.com/python/cpython/blob/v3.11.0/Lib/zipfile.py#L1828-L1831\n\nFWIW, I think we might be able to pass the array directly on py3:\n\n $ python\n Python 3.10.6 (main, Aug  2 2022, 00:00:00) [GCC 12.1.1 20220507 (Red Hat 12.1.1-1)] on linux\n Type \"help\", \"copyright\", \"credits\" or \"license\" for more information.\n \u003e\u003e\u003e import io, array\n \u003e\u003e\u003e b\u003dio.BytesIO()\n \u003e\u003e\u003e b.write(array.array(\u0027H\u0027, range(5)))\n 10\n \u003e\u003e\u003e b.getvalue()\n b\u0027\\x00\\x00\\x01\\x00\\x02\\x00\\x03\\x00\\x04\\x00\u0027\n\n... but not py2:\n\n $ python2\n Python 2.7.18 (default, Jun  9 2022, 00:00:00) \n [GCC 12.1.1 20220507 (Red Hat 12.1.1-1)] on linux2\n Type \"help\", \"copyright\", \"credits\" or \"license\" for more information.\n \u003e\u003e\u003e import io, array\n \u003e\u003e\u003e b\u003dio.BytesIO()\n \u003e\u003e\u003e b.write(array.array(\u0027H\u0027, range(5)))\n Traceback (most recent call last):\n   File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\n TypeError: \u0027array.array\u0027 does not have the buffer interface","commit_id":"9fea3830637e1b42d76bb10ef8b5202cc009e7a3"}]}