)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9c630e530477f2e833e77839c97f37ba00328773","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"c938ad5c_4a2758ad","updated":"2023-05-04 16:05:09.000000000","message":"love seeing those negative line # diffs!  keep going!","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f77eaf922db47bb942d8637ee786b2e44ab0aaa8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4de6e0ed_91f9e445","in_reply_to":"c938ad5c_4a2758ad","updated":"2023-05-04 17:12:34.000000000","message":"IDK -- net -17 doesn\u0027t inspire a bunch of confidence that this is the right direction yet. Maybe if I keep going, though...","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"566c31dc1a6aa79add83cb50b30da80d0d2b972c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0e6d6439_0a5a611a","updated":"2023-05-08 06:48:57.000000000","message":"I really like the abstraction. Moving the seek, tell, etc up to the file reader/writer means the RingReader/Writer gets to focus on it\u0027s own important bits.\n\nHaving a zlib_fp and raw_fp in the RingReader/Writer classes did make thing more confusing so I think this is a win.\n\nI personally don\u0027t mind taking a filelike, as we can use the classmethod to simply open it in a context manager, but being able to use a filelike means we can easily use other datastructors in testing or in memory potentually as required.\n\nI have only looked at code, haven\u0027t tested it, so it hasn\u0027t got a +2 yet.","commit_id":"7b7beb542a3518ce4266e670944b4b5387f28bbb"}],"swift/common/ring/io.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"ef43adddd907c1b8d943807cd033f0d0089d943a","unresolved":true,"context_lines":[{"line_number":260,"context_line":"        return self"},{"line_number":261,"context_line":""},{"line_number":262,"context_line":"    def __exit__(self, *args):"},{"line_number":263,"context_line":"        self.close()"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def readinto(self, buffer):"},{"line_number":266,"context_line":"        # Retained so pickle is happy on python 3.8.0 and 3.8.1 (i.e., versions"}],"source_content_type":"text/x-python","patch_set":1,"id":"d09fbabb_22138fbc","line":263,"updated":"2023-05-04 16:03:20.000000000","message":"This assumption of ownership rubs me the wrong way -- probably better to have something like\n```\n@classmethod\n@contextmanager\ndef open(cls, filename):\n    with open(filename, \u0027rb\u0027) as fp:\n        yield cls(fp)\n```\nand ditch the `close` methods.","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7a4b2ccdea2efe17935834269eaef2f9b37e826b","unresolved":true,"context_lines":[{"line_number":262,"context_line":"    def __exit__(self, *args):"},{"line_number":263,"context_line":"        self.close()"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def readinto(self, buffer):"},{"line_number":266,"context_line":"        # Retained so pickle is happy on python 3.8.0 and 3.8.1 (i.e., versions"},{"line_number":267,"context_line":"        # released with https://github.com/python/cpython/commit/91f4380c but"},{"line_number":268,"context_line":"        # neither https://github.com/python/cpython/commit/9f37872e nor"}],"source_content_type":"text/x-python","patch_set":1,"id":"2e4ae7c2_c8f1c5ce","line":265,"updated":"2023-05-03 23:09:36.000000000","message":"This should probably move up into `ZlibReader`","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9c630e530477f2e833e77839c97f37ba00328773","unresolved":true,"context_lines":[{"line_number":262,"context_line":"    def __exit__(self, *args):"},{"line_number":263,"context_line":"        self.close()"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def readinto(self, buffer):"},{"line_number":266,"context_line":"        # Retained so pickle is happy on python 3.8.0 and 3.8.1 (i.e., versions"},{"line_number":267,"context_line":"        # released with https://github.com/python/cpython/commit/91f4380c but"},{"line_number":268,"context_line":"        # neither https://github.com/python/cpython/commit/9f37872e nor"}],"source_content_type":"text/x-python","patch_set":1,"id":"c36b4e2d_c8793c0f","line":265,"in_reply_to":"2e4ae7c2_c8f1c5ce","updated":"2023-05-04 16:05:09.000000000","message":"ok, so ZlibReader was only ever used from with-in this class and now this class *is* a ZlibReader?  I think this smells like there is only one concrete class here and it shouldn\u0027t matter much where you put methods.","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f77eaf922db47bb942d8637ee786b2e44ab0aaa8","unresolved":true,"context_lines":[{"line_number":262,"context_line":"    def __exit__(self, *args):"},{"line_number":263,"context_line":"        self.close()"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def readinto(self, buffer):"},{"line_number":266,"context_line":"        # Retained so pickle is happy on python 3.8.0 and 3.8.1 (i.e., versions"},{"line_number":267,"context_line":"        # released with https://github.com/python/cpython/commit/91f4380c but"},{"line_number":268,"context_line":"        # neither https://github.com/python/cpython/commit/9f37872e nor"}],"source_content_type":"text/x-python","patch_set":1,"id":"e935bfdc_94692365","line":265,"in_reply_to":"c36b4e2d_c8793c0f","updated":"2023-05-04 17:12:34.000000000","message":"I think there\u0027s still a case to be made that it\u0027s worth us trying to isolate the zlib-stream shenanigans from the ring-specific features we need.","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4c3c8f3f3d8d340a2b132b798b4ea90131615f42","unresolved":true,"context_lines":[{"line_number":262,"context_line":"    def __exit__(self, *args):"},{"line_number":263,"context_line":"        self.close()"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def readinto(self, buffer):"},{"line_number":266,"context_line":"        # Retained so pickle is happy on python 3.8.0 and 3.8.1 (i.e., versions"},{"line_number":267,"context_line":"        # released with https://github.com/python/cpython/commit/91f4380c but"},{"line_number":268,"context_line":"        # neither https://github.com/python/cpython/commit/9f37872e nor"}],"source_content_type":"text/x-python","patch_set":1,"id":"903f5d41_8a896af5","line":265,"in_reply_to":"e935bfdc_94692365","updated":"2023-05-04 20:11:11.000000000","message":"Arguably, I should enforce similar separation for `RingWriter`... so `RingWriter`, with the `section` and `write_*` helpers, is-a `ZlibWriter` which has the `tell`, `write`, `flush`, `close`, and `set_compression_level` methods...","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"ef43adddd907c1b8d943807cd033f0d0089d943a","unresolved":true,"context_lines":[{"line_number":368,"context_line":""},{"line_number":369,"context_line":"    This has a few key features on top of a standard ``GzipFile``:"},{"line_number":370,"context_line":""},{"line_number":371,"context_line":"    * Atomic writes using a temporary file"},{"line_number":372,"context_line":"    * Helpers for writing length-value encoded BLOBs"},{"line_number":373,"context_line":"    * The ability to define named sections which will be written as"},{"line_number":374,"context_line":"      an index at the end of the file"}],"source_content_type":"text/x-python","patch_set":1,"id":"7bc352ac_8a0efa8b","line":371,"range":{"start_line":371,"start_character":6,"end_line":371,"end_character":42},"updated":"2023-05-04 16:03:20.000000000","message":"If we take `RingWriter` in a similar direction of just accepting a writable file-like, we\u0027d have to ditch this part. Or rather, add some new `AtomicFileWriter` that we then pass as the file-like...\n\nMaybe that\u0027d be good to have anyway? Could probably be pulled out of obj/diskfile.py so ring-writing would get `O_TMPFILE` support if available...","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"4c3c8f3f3d8d340a2b132b798b4ea90131615f42","unresolved":true,"context_lines":[{"line_number":368,"context_line":""},{"line_number":369,"context_line":"    This has a few key features on top of a standard ``GzipFile``:"},{"line_number":370,"context_line":""},{"line_number":371,"context_line":"    * Atomic writes using a temporary file"},{"line_number":372,"context_line":"    * Helpers for writing length-value encoded BLOBs"},{"line_number":373,"context_line":"    * The ability to define named sections which will be written as"},{"line_number":374,"context_line":"      an index at the end of the file"}],"source_content_type":"text/x-python","patch_set":1,"id":"d4b9682e_b83bd9ca","line":371,"range":{"start_line":371,"start_character":6,"end_line":371,"end_character":42},"in_reply_to":"7bc352ac_8a0efa8b","updated":"2023-05-04 20:11:11.000000000","message":"Alternatively, I can move that *one* bit to a new `RingWriter.open` class method...","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"3f3775ac1befb4fd429fa4e8bb648f4cbeeca9e6","unresolved":true,"context_lines":[{"line_number":73,"context_line":"        return self.fp.tell()"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def close(self):"},{"line_number":76,"context_line":"        return self.fp.close()"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    def _decompress_from_buffer(self, offset):"},{"line_number":79,"context_line":"        if offset \u003c 0:"}],"source_content_type":"text/x-python","patch_set":2,"id":"417e885f_b246a4bd","line":76,"updated":"2023-05-04 23:54:03.000000000","message":"Hold up -- I meant to drop this! We don\u0027t own the file-like, we shouldn\u0027t be closing it!","commit_id":"7b7beb542a3518ce4266e670944b4b5387f28bbb"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"566c31dc1a6aa79add83cb50b30da80d0d2b972c","unresolved":true,"context_lines":[{"line_number":73,"context_line":"        return self.fp.tell()"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    def close(self):"},{"line_number":76,"context_line":"        return self.fp.close()"},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"    def _decompress_from_buffer(self, offset):"},{"line_number":79,"context_line":"        if offset \u003c 0:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5d0f5c7d_163fdfd2","line":76,"in_reply_to":"417e885f_b246a4bd","updated":"2023-05-08 06:48:57.000000000","message":"+1","commit_id":"7b7beb542a3518ce4266e670944b4b5387f28bbb"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"566c31dc1a6aa79add83cb50b30da80d0d2b972c","unresolved":true,"context_lines":[{"line_number":267,"context_line":""},{"line_number":268,"context_line":"    @classmethod"},{"line_number":269,"context_line":"    @contextlib.contextmanager"},{"line_number":270,"context_line":"    def open(cls, filename):"},{"line_number":271,"context_line":"        \"\"\""},{"line_number":272,"context_line":"        Open the ring file ``filename``"},{"line_number":273,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"943f1f04_5c452d21","line":270,"range":{"start_line":270,"start_character":4,"end_line":270,"end_character":28},"updated":"2023-05-08 06:48:57.000000000","message":"The classmethod/context open is in the ZlibWriter class not the RingWriter, so this should move up to the parent class and be consistant. (or visa vera)","commit_id":"7b7beb542a3518ce4266e670944b4b5387f28bbb"}],"swift/common/ring/ring.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7a4b2ccdea2efe17935834269eaef2f9b37e826b","unresolved":true,"context_lines":[{"line_number":258,"context_line":"        :param bool metadata_only: If True, only load `devs` and `part_shift`."},{"line_number":259,"context_line":"        :returns: A RingData instance containing the loaded data."},{"line_number":260,"context_line":"        \"\"\""},{"line_number":261,"context_line":"        with open(filename, \u0027rb\u0027) as fp, RingReader(fp) as reader:"},{"line_number":262,"context_line":"            if reader.version not in RING_CODECS:"},{"line_number":263,"context_line":"                raise Exception(\u0027Unknown ring format version %d\u0027 %"},{"line_number":264,"context_line":"                                reader.version)"}],"source_content_type":"text/x-python","patch_set":1,"id":"bc4f279c_b725da4c","line":261,"updated":"2023-05-03 23:09:36.000000000","message":"The way I\u0027ve got things, this can safely be\n```\nwith RingReader(open(filename, \u0027rb\u0027)) as reader:\n```\nbut it\u0027d surely be even better with a `@classmethod` like\n```\nwith RingReader.open(filename) as reader:\n```","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f77eaf922db47bb942d8637ee786b2e44ab0aaa8","unresolved":true,"context_lines":[{"line_number":258,"context_line":"        :param bool metadata_only: If True, only load `devs` and `part_shift`."},{"line_number":259,"context_line":"        :returns: A RingData instance containing the loaded data."},{"line_number":260,"context_line":"        \"\"\""},{"line_number":261,"context_line":"        with open(filename, \u0027rb\u0027) as fp, RingReader(fp) as reader:"},{"line_number":262,"context_line":"            if reader.version not in RING_CODECS:"},{"line_number":263,"context_line":"                raise Exception(\u0027Unknown ring format version %d\u0027 %"},{"line_number":264,"context_line":"                                reader.version)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5e70973c_b22fb286","line":261,"in_reply_to":"0333b807_6cb0ebea","updated":"2023-05-04 17:12:34.000000000","message":"\u003e I think having RingReader take an open file is kinda weird\n\nIDK, I\u0027ve definitely seen it in other APIs and appreciated it:\n\n* https://docs.python.org/3/library/gzip.html#gzip.GzipFile takes a `fileobj` arg\n* https://docs.python.org/3/library/tarfile.html#tarfile.TarFile takes a `fileobj`\n* https://docs.python.org/3/library/hashlib.html#hashlib.file_digest *only* takes a `fileobj`\n\nIt gets a little messy for the `RingReader` in particular (since it requires a *seekable* file-like; you likely *wouldn\u0027t* want it to actually be backed by a socket or something), but if I keep on this and get tests to not touch disk, it seems like a win.","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9c630e530477f2e833e77839c97f37ba00328773","unresolved":true,"context_lines":[{"line_number":258,"context_line":"        :param bool metadata_only: If True, only load `devs` and `part_shift`."},{"line_number":259,"context_line":"        :returns: A RingData instance containing the loaded data."},{"line_number":260,"context_line":"        \"\"\""},{"line_number":261,"context_line":"        with open(filename, \u0027rb\u0027) as fp, RingReader(fp) as reader:"},{"line_number":262,"context_line":"            if reader.version not in RING_CODECS:"},{"line_number":263,"context_line":"                raise Exception(\u0027Unknown ring format version %d\u0027 %"},{"line_number":264,"context_line":"                                reader.version)"}],"source_content_type":"text/x-python","patch_set":1,"id":"0333b807_6cb0ebea","line":261,"in_reply_to":"bc4f279c_b725da4c","updated":"2023-05-04 16:05:09.000000000","message":"i think `with open_ring(filename) as reader` should be fine as far as pythonic apis, I think what was there looked pretty good?\n\nI think having RingReader take an open file is kinda weird since OOO on the surface is all about abstracting behaviors by keeping state internal to the object.","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"}],"test/unit/common/ring/test_io.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"896eff89666184d87c117128297089d9f4f73673","unresolved":true,"context_lines":[{"line_number":24,"context_line":""},{"line_number":25,"context_line":"from swift.common.ring.io import IndexEntry, RingReader, RingWriter"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"from test.unit import with_tempdir"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"class TestRoundTrip(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":1,"id":"e67040f8_1a5a5ab0","line":27,"updated":"2023-05-03 22:57:21.000000000","message":"The general goal I\u0027ve got in mind is to get rid of this in favor of using `io.BytesIO` in-memory, but it\u0027ll require similar sorts of changes to `RingWriter`...","commit_id":"ec83e703bd6231cb9e1a21f8d7398ed308664662"}]}