)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"03f824a77c0fc7e81f7a42a6aaace660c4add9d0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e07dfe61_035e35b4","updated":"2023-09-13 09:45:15.000000000","message":"I think this would be acceptable as it is, but better if we avoided any notion of extension bleeding out to the object server interface as even a kwarg in DiskFile.writer() i.e. add a _writer shim method or similar","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"90c7b88379c4b0cd8ace437278c77162d3900c6b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"2032bc62_64baf88c","updated":"2023-09-12 18:17:51.000000000","message":"Let\u0027s see what Zuul thinks of it -- I might rebase https://review.opendev.org/c/openstack/swift/+/891382 on top.","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"699f856db16d11e3331a34cb11ac376a5728b734","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"01f6051e_e8cf76f1","updated":"2023-09-12 19:35:14.000000000","message":"df interfaces are so weird - I think there\u0027s tons of unclear boundaries between what\u0027s public and what\u0027s private\n\nesentially the only guidance we have is duck typing - whatever the object server asks it to do is what a working df interface has to support.  Everything else that it does is just internal details.","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6c89576cc3469d3848c51ed36f5b85059b5d504a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"d61aab9f_7b0a96f7","updated":"2023-09-22 15:16:06.000000000","message":"This is a nice and likely necessary cleanup/clarification.\n\nIt\u0027s unfortunate that the interface for BaseDiskFileWriter changes (potentially impacting a third party subclass implementation), but the fact that BaseDiskFile was setting an underscore attribute of BaseDiskFileWriter was already a strong hint that the two class implementations were tightly coupled.","commit_id":"4051053be48d20ad2755e211ecb7e9681a630e4d"}],"swift/obj/diskfile.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"90c7b88379c4b0cd8ace437278c77162d3900c6b","unresolved":true,"context_lines":[{"line_number":1811,"context_line":"        # Internal attributes"},{"line_number":1812,"context_line":"        self._upload_size \u003d 0"},{"line_number":1813,"context_line":"        self._last_sync \u003d 0"},{"line_number":1814,"context_line":"        self._extension \u003d \u0027.data\u0027"},{"line_number":1815,"context_line":"        self._put_succeeded \u003d False"},{"line_number":1816,"context_line":""},{"line_number":1817,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":1,"id":"e07f1a8e_82fba94c","side":"PARENT","line":1814,"updated":"2023-09-12 18:17:51.000000000","message":"This was what started really bugging me: we list `_extension` under \"Internal attributes\", but then go manipulating this pseudo-private attribute over in `BaseDiskFile.write_metadata` and `BaseDiskFile.delete`!","commit_id":"e2dca715038ddcda072841b413db73569494c31c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"699f856db16d11e3331a34cb11ac376a5728b734","unresolved":true,"context_lines":[{"line_number":3020,"context_line":"                            errors as the `create()` method."},{"line_number":3021,"context_line":"        \"\"\""},{"line_number":3022,"context_line":"        with self.create() as writer:"},{"line_number":3023,"context_line":"            writer._extension \u003d \u0027.meta\u0027"},{"line_number":3024,"context_line":"            writer.put(metadata)"},{"line_number":3025,"context_line":""},{"line_number":3026,"context_line":"    def delete(self, timestamp):"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f1e8a03_0c72553d","side":"PARENT","line":3023,"updated":"2023-09-12 19:35:14.000000000","message":"oh ok, that\u0027s interesting","commit_id":"e2dca715038ddcda072841b413db73569494c31c"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"90c7b88379c4b0cd8ace437278c77162d3900c6b","unresolved":true,"context_lines":[{"line_number":1798,"context_line":"    \"\"\""},{"line_number":1799,"context_line":""},{"line_number":1800,"context_line":"    def __init__(self, name, datadir, size, bytes_per_sync, diskfile,"},{"line_number":1801,"context_line":"                 next_part_power, extension\u003d\u0027.data\u0027):"},{"line_number":1802,"context_line":"        # Parameter tracking"},{"line_number":1803,"context_line":"        self._name \u003d name"},{"line_number":1804,"context_line":"        self._datadir \u003d datadir"}],"source_content_type":"text/x-python","patch_set":1,"id":"d5cbb4a2_60e4650c","line":1801,"updated":"2023-09-12 18:17:51.000000000","message":"It\u0027s not clear to me whether this strictly *needs* to be a kwarg -- maybe it\u0027d be OK without a default? Maybe even put it closer to `size`? I\u0027m not clear on how much we need to worry about external consumers of this API.","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6c89576cc3469d3848c51ed36f5b85059b5d504a","unresolved":false,"context_lines":[{"line_number":1798,"context_line":"    \"\"\""},{"line_number":1799,"context_line":""},{"line_number":1800,"context_line":"    def __init__(self, name, datadir, size, bytes_per_sync, diskfile,"},{"line_number":1801,"context_line":"                 next_part_power, extension\u003d\u0027.data\u0027):"},{"line_number":1802,"context_line":"        # Parameter tracking"},{"line_number":1803,"context_line":"        self._name \u003d name"},{"line_number":1804,"context_line":"        self._datadir \u003d datadir"}],"source_content_type":"text/x-python","patch_set":1,"id":"40762ddb_70263e91","line":1801,"in_reply_to":"0bcb8815_2f38e5e6","updated":"2023-09-22 15:16:06.000000000","message":"Done","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"03f824a77c0fc7e81f7a42a6aaace660c4add9d0","unresolved":true,"context_lines":[{"line_number":1798,"context_line":"    \"\"\""},{"line_number":1799,"context_line":""},{"line_number":1800,"context_line":"    def __init__(self, name, datadir, size, bytes_per_sync, diskfile,"},{"line_number":1801,"context_line":"                 next_part_power, extension\u003d\u0027.data\u0027):"},{"line_number":1802,"context_line":"        # Parameter tracking"},{"line_number":1803,"context_line":"        self._name \u003d name"},{"line_number":1804,"context_line":"        self._datadir \u003d datadir"}],"source_content_type":"text/x-python","patch_set":1,"id":"0bcb8815_2f38e5e6","line":1801,"in_reply_to":"d5cbb4a2_60e4650c","updated":"2023-09-13 09:45:15.000000000","message":"IMHO a kwarg is OK and then we don\u0027t have to worry about external consumers","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"03f824a77c0fc7e81f7a42a6aaace660c4add9d0","unresolved":true,"context_lines":[{"line_number":1809,"context_line":"        self._bytes_per_sync \u003d bytes_per_sync"},{"line_number":1810,"context_line":"        self._diskfile \u003d diskfile"},{"line_number":1811,"context_line":"        self.next_part_power \u003d next_part_power"},{"line_number":1812,"context_line":"        self._extension \u003d extension"},{"line_number":1813,"context_line":""},{"line_number":1814,"context_line":"        # Internal attributes"},{"line_number":1815,"context_line":"        self._upload_size \u003d 0"}],"source_content_type":"text/x-python","patch_set":1,"id":"6df01b08_b9376364","line":1812,"updated":"2023-09-13 09:45:15.000000000","message":"off-topic: ideally I\u0027d make this more abstract - rather than DiskFileWriter knowing about file extensions, it needs to know about file type (so could be represented by constants DATA, META or TOMBSTONE). File naming is delegated to the DiskFileManager via make_on_disk_filename() which would map the constant to an extension.\n\nBut, we\u0027re not in the ideal world, and I think it\u0027s too late and not worth thinking about. It just bugs me how many string literals get passed around.","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6c89576cc3469d3848c51ed36f5b85059b5d504a","unresolved":false,"context_lines":[{"line_number":1809,"context_line":"        self._bytes_per_sync \u003d bytes_per_sync"},{"line_number":1810,"context_line":"        self._diskfile \u003d diskfile"},{"line_number":1811,"context_line":"        self.next_part_power \u003d next_part_power"},{"line_number":1812,"context_line":"        self._extension \u003d extension"},{"line_number":1813,"context_line":""},{"line_number":1814,"context_line":"        # Internal attributes"},{"line_number":1815,"context_line":"        self._upload_size \u003d 0"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a9c2359_633a38fe","line":1812,"in_reply_to":"6df01b08_b9376364","updated":"2023-09-22 15:16:06.000000000","message":"Ack","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"90c7b88379c4b0cd8ace437278c77162d3900c6b","unresolved":true,"context_lines":[{"line_number":2984,"context_line":"        self._fp \u003d None"},{"line_number":2985,"context_line":"        return dr"},{"line_number":2986,"context_line":""},{"line_number":2987,"context_line":"    def writer(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2988,"context_line":"        return self.writer_cls(self._name, self._datadir, size,"},{"line_number":2989,"context_line":"                               self._bytes_per_sync, self,"},{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f629005_1318084e","line":2987,"updated":"2023-09-12 18:17:51.000000000","message":"There should probably be a similar debate about whether this should be a kwarg...","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"699f856db16d11e3331a34cb11ac376a5728b734","unresolved":true,"context_lines":[{"line_number":2984,"context_line":"        self._fp \u003d None"},{"line_number":2985,"context_line":"        return dr"},{"line_number":2986,"context_line":""},{"line_number":2987,"context_line":"    def writer(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2988,"context_line":"        return self.writer_cls(self._name, self._datadir, size,"},{"line_number":2989,"context_line":"                               self._bytes_per_sync, self,"},{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"}],"source_content_type":"text/x-python","patch_set":1,"id":"fa51bcb7_f4679d09","line":2987,"in_reply_to":"7f629005_1318084e","updated":"2023-09-12 19:35:14.000000000","message":"the object server calls this for PUTs - DELETEs and POSTs use the helpers.\n\nif someone has a DF that works w/o the caller sending this kwarg we shouldn\u0027t break it, if they inhret from some of this code and want to support new swift they\u0027d have to update the signatures to match (and maybe ignore) the new kwarg)","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"454816ef074d2c052bcf37f2d7f68e4b28d8e3ab","unresolved":false,"context_lines":[{"line_number":2984,"context_line":"        self._fp \u003d None"},{"line_number":2985,"context_line":"        return dr"},{"line_number":2986,"context_line":""},{"line_number":2987,"context_line":"    def writer(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2988,"context_line":"        return self.writer_cls(self._name, self._datadir, size,"},{"line_number":2989,"context_line":"                               self._bytes_per_sync, self,"},{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"}],"source_content_type":"text/x-python","patch_set":1,"id":"48d58c25_4f8694d8","line":2987,"in_reply_to":"e8898026_a22356e6","updated":"2023-09-19 17:05:12.000000000","message":"Done","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"03f824a77c0fc7e81f7a42a6aaace660c4add9d0","unresolved":true,"context_lines":[{"line_number":2984,"context_line":"        self._fp \u003d None"},{"line_number":2985,"context_line":"        return dr"},{"line_number":2986,"context_line":""},{"line_number":2987,"context_line":"    def writer(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2988,"context_line":"        return self.writer_cls(self._name, self._datadir, size,"},{"line_number":2989,"context_line":"                               self._bytes_per_sync, self,"},{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"}],"source_content_type":"text/x-python","patch_set":1,"id":"e8898026_a22356e6","line":2987,"in_reply_to":"fa51bcb7_f4679d09","updated":"2023-09-13 09:45:15.000000000","message":"IMO the object server should not know about file extensions and therefore should not be required to pass \u0027.data\u0027 to this method. So extension has to be a keyword arg.\n\nOr we even add a shim ``_writer(self, size, extension)`` method and call through to it, so there is not even suggestion that the object server might ever pass an extension kwarg i.e.:\n\n```\ndef writer(self, size\u003dNone):\n    return self._writer(size, \u0027.data\u0027)\n```","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"699f856db16d11e3331a34cb11ac376a5728b734","unresolved":true,"context_lines":[{"line_number":2987,"context_line":"    def writer(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2988,"context_line":"        return self.writer_cls(self._name, self._datadir, size,"},{"line_number":2989,"context_line":"                               self._bytes_per_sync, self,"},{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"},{"line_number":2991,"context_line":""},{"line_number":2992,"context_line":"    @contextmanager"},{"line_number":2993,"context_line":"    def create(self, size\u003dNone, extension\u003d\u0027.data\u0027):"}],"source_content_type":"text/x-python","patch_set":1,"id":"dca02611_05f6f899","line":2990,"updated":"2023-09-12 19:35:14.000000000","message":"If it wasn\u0027t for the fact that the internal create helper re-uses this public interface I think you could always force it to \u0027.data\u0027 and the obj-server would be happy.","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"90c7b88379c4b0cd8ace437278c77162d3900c6b","unresolved":true,"context_lines":[{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"},{"line_number":2991,"context_line":""},{"line_number":2992,"context_line":"    @contextmanager"},{"line_number":2993,"context_line":"    def create(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2994,"context_line":"        \"\"\""},{"line_number":2995,"context_line":"        Context manager to create a file. We create a temporary file first, and"},{"line_number":2996,"context_line":"        then return a DiskFileWriter object to encapsulate the state."}],"source_content_type":"text/x-python","patch_set":1,"id":"7d1e80bb_e5cf2056","line":2993,"updated":"2023-09-12 18:17:51.000000000","message":"But then I think this one almost certainly *does* need to be a kwarg with a default of `.data`","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"699f856db16d11e3331a34cb11ac376a5728b734","unresolved":true,"context_lines":[{"line_number":2990,"context_line":"                               self.next_part_power, extension\u003dextension)"},{"line_number":2991,"context_line":""},{"line_number":2992,"context_line":"    @contextmanager"},{"line_number":2993,"context_line":"    def create(self, size\u003dNone, extension\u003d\u0027.data\u0027):"},{"line_number":2994,"context_line":"        \"\"\""},{"line_number":2995,"context_line":"        Context manager to create a file. We create a temporary file first, and"},{"line_number":2996,"context_line":"        then return a DiskFileWriter object to encapsulate the state."}],"source_content_type":"text/x-python","patch_set":1,"id":"10102754_ab1f58ca","line":2993,"in_reply_to":"7d1e80bb_e5cf2056","updated":"2023-09-12 19:35:14.000000000","message":"idk, i think this is mostly an internal interface?  used as a convinece to wrap the open/close and you just need to call one method on the df_writer object?  the obj server doesn\u0027t use create I don\u0027t think.","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"03f824a77c0fc7e81f7a42a6aaace660c4add9d0","unresolved":true,"context_lines":[{"line_number":3007,"context_line":"                          defaults to ``.data``"},{"line_number":3008,"context_line":"        :raises DiskFileNoSpace: if a size is specified and allocation fails"},{"line_number":3009,"context_line":"        \"\"\""},{"line_number":3010,"context_line":"        dfw \u003d self.writer(size, extension\u003dextension)"},{"line_number":3011,"context_line":"        try:"},{"line_number":3012,"context_line":"            yield dfw.open()"},{"line_number":3013,"context_line":"        finally:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c237153c_2bf3894b","line":3010,"updated":"2023-09-13 09:45:15.000000000","message":"(see above) here we might write\n\n```\ndfw\u003d self._writer(size, extension)\n```","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"454816ef074d2c052bcf37f2d7f68e4b28d8e3ab","unresolved":false,"context_lines":[{"line_number":3007,"context_line":"                          defaults to ``.data``"},{"line_number":3008,"context_line":"        :raises DiskFileNoSpace: if a size is specified and allocation fails"},{"line_number":3009,"context_line":"        \"\"\""},{"line_number":3010,"context_line":"        dfw \u003d self.writer(size, extension\u003dextension)"},{"line_number":3011,"context_line":"        try:"},{"line_number":3012,"context_line":"            yield dfw.open()"},{"line_number":3013,"context_line":"        finally:"}],"source_content_type":"text/x-python","patch_set":1,"id":"b48dbfc5_f959a1dd","line":3010,"in_reply_to":"c237153c_2bf3894b","updated":"2023-09-19 17:05:12.000000000","message":"Done","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"699f856db16d11e3331a34cb11ac376a5728b734","unresolved":true,"context_lines":[{"line_number":3023,"context_line":"        :raises DiskFileError: this implementation will raise the same"},{"line_number":3024,"context_line":"                            errors as the `create()` method."},{"line_number":3025,"context_line":"        \"\"\""},{"line_number":3026,"context_line":"        with self.create(extension\u003d\u0027.meta\u0027) as writer:"},{"line_number":3027,"context_line":"            writer.put(metadata)"},{"line_number":3028,"context_line":""},{"line_number":3029,"context_line":"    def delete(self, timestamp):"}],"source_content_type":"text/x-python","patch_set":1,"id":"5ae5c8d6_1280fe38","line":3026,"updated":"2023-09-12 19:35:14.000000000","message":"maybe use a self._writer(\u0027.meta\u0027) kind of pattern?","commit_id":"124720e8e34fe4924ed1c1e3a6ecb59ebb035194"}]}