)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"f842c667b9cd5effb1037e39e4596c5cbfa94780","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"44ade880_0144dc5c","updated":"2025-10-17 15:28:13.000000000","message":"expected test for reviewing:\n\nswift/test/unit/obj/test_updater.py::TestObjectUpdater::test_per_container_rate_limit_unsent_deferrals\nand\nswift/test/unit/obj/test_updater.py::TestObjectUpdater::test_per_container_rate_limit_defer_3_skip_1","commit_id":"8ebbf7a75f4c1fca6c34ab96ef2227dfead86c41"},{"author":{"_account_id":34892,"name":"ASHWIN A NAIR","display_name":"indianwhocodes","email":"nairashwin952013@gmail.com","username":"indianwhocodes","status":"Nvidia"},"change_message_id":"151ab04e4b6f4e04e6a3e2fb266abbdb1d7a7d0f","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":21,"id":"34ca505c_84c3f07c","updated":"2025-10-28 19:06:34.000000000","message":"We gotta cleanup the `pep8` errors unless you\u0027re asking a wip review","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"08c8402c_0cd12803","updated":"2025-10-28 19:17:42.000000000","message":"not a full review, but hopefully some pointers to help you move forwards.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"07965bf66580425f6ea22303313a824880be0d12","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"acc3c12f_efa5df24","in_reply_to":"34ca505c_84c3f07c","updated":"2025-11-04 15:34:02.000000000","message":"Yep, a wip review. Still have to add tests and all. I will change the name to specify that!","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"}],"swift/obj/updater.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":169,"context_line":"                # no need to ratelimit, just return next update"},{"line_number":170,"context_line":"                return update_ctx"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"            labels \u003d self._get_acco_from_update(update_ctx)"},{"line_number":173,"context_line":"            self.stats.deferrals +\u003d 1"},{"line_number":174,"context_line":"            self.logger.increment(\"deferrals\")"},{"line_number":175,"context_line":""}],"source_content_type":"text/x-python","patch_set":21,"id":"ebe19609_913c0a39","line":172,"updated":"2025-10-28 19:17:42.000000000","message":"these labels are not going to be correct for each of the following events., because sometimes we skip the current update_ctx and sometimes we skip a previously deferred update which would have been for a different account/container.\n\nHere\u0027s what\u0027s happening:\n* for each update yielded from self.iterator, can it be sent immediately? if so then return at line 170, if not then it is deferred\n\n* if deferred then the yielded update suffers one of two fates: \nIf max_deferred_elements is \u003e 0 then the yielded update will be added to a bucket (which is a queue) to potentially be sent later; as a side effect, a previously queued update may be dropped from the queues (\"skipped\") to make space for the yielded update.\n\nIf max_deferred_elements \u003c\u003d0 (deferrals are effectively disabled) then the yielded update is immediately skipped.\n\nThat is the end of processing for the yielded update. Everything from line 198 onwards is processing previously queued updates taken from the buckets.\n\nSo, up to line 198 one or more \"events\" may have occurred:\n* an update was deferred\n* the deferred update was immediately skipped (deferrals disable)\n* another previously deferred update was skipped (deferral queues full) - this will need different a/c labels\n\nEvery time one of those events occurs we want to *increment* a labeled counter by 1.\n\nThen, after line 198 we try to send a previously queued update from the existing queued buckets. If this is possible then there is a \u0027deferred_drain\u0027 event, whose labels will need to be whatever the a/c of the de-queued update is.\n\nFinally, the \u0027undrained_skips\u0027 is different: it is reporting an aggregate and so cannot be labeled with account/container.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":183,"context_line":"                    oldest_deferred_bucket.deque.popleft()"},{"line_number":184,"context_line":"                    self.stats.skips +\u003d 1"},{"line_number":185,"context_line":"                    self.logger.increment(\"skips\")"},{"line_number":186,"context_line":"                    happen \u003d \u0027deferred_skips\u0027"},{"line_number":187,"context_line":"                # append the update to the bucket\u0027s queue and append the bucket"},{"line_number":188,"context_line":"                # to the queue of deferred buckets"},{"line_number":189,"context_line":"                # note: buckets may have multiple entries in deferred_buckets,"}],"source_content_type":"text/x-python","patch_set":21,"id":"c7887b48_280b0aad","line":186,"range":{"start_line":186,"start_character":20,"end_line":186,"end_character":26},"updated":"2025-10-28 19:17:42.000000000","message":"``happen`` is going to become labels[\u0027event\u0027] so calling the variable ``event`` removes one mental mapping.\n\nHowever, given that the account/container lables differe here, you may find that just declaring the entire ``labels \u003d {...}``  wherever you have ``happen`` is easier and clearer.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":195,"context_line":"                self.logger.increment(\"skips\")"},{"line_number":196,"context_line":"                happen \u003d \u0027deferred_skips\u0027"},{"line_number":197,"context_line":""},{"line_number":198,"context_line":"        if self.buckets_ordered_by_readiness is None:"},{"line_number":199,"context_line":"            # initialise a queue of those buckets with deferred elements;"},{"line_number":200,"context_line":"            # buckets are queued in the chronological order in which they are"},{"line_number":201,"context_line":"            # ready to serve an element"}],"source_content_type":"text/x-python","patch_set":21,"id":"6216e13a_998863cf","line":198,"updated":"2025-10-28 19:17:42.000000000","message":"this is the beginning of draining (although the comment at line 213 unhelpfully suggests otherwise). It\u0027s at least the preparatory work for draining.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":204,"context_line":"                if bucket:"},{"line_number":205,"context_line":"                    self.buckets_ordered_by_readiness.put(bucket)"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"        if happen:"},{"line_number":208,"context_line":"            labels[\u0027event\u0027] \u003d \u0027deferred_skips\u0027"},{"line_number":209,"context_line":"            self.statsd.update_stats(\u0027swift_object_updater\u0027,"},{"line_number":210,"context_line":"                                     self.stats.skips,"}],"source_content_type":"text/x-python","patch_set":21,"id":"3baa4b34_504c7da2","line":207,"updated":"2025-10-28 19:17:42.000000000","message":"this sortof belongs before line 198: we\u0027re done with adding to the deferred bucket queue, so emit a stats reporting what the outcome was.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":207,"context_line":"        if happen:"},{"line_number":208,"context_line":"            labels[\u0027event\u0027] \u003d \u0027deferred_skips\u0027"},{"line_number":209,"context_line":"            self.statsd.update_stats(\u0027swift_object_updater\u0027,"},{"line_number":210,"context_line":"                                     self.stats.skips,"},{"line_number":211,"context_line":"                                     labels\u003dlabels)"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"        # now drain the buckets..."}],"source_content_type":"text/x-python","patch_set":21,"id":"f2fcde9d_d7341194","line":210,"range":{"start_line":210,"start_character":37,"end_line":210,"end_character":53},"updated":"2025-10-28 19:17:42.000000000","message":"-1. \n``self.stats.skips`` is (IIUC) the running total of skips in this cycle. ``update_stats`` is incrementing a counter, so it\u0027s semantic is \"an occurrence of this event has happened\" and NOT \"here\u0027s the running total of haw many of these events have happened so far\".\nSo, we just want to increment with counter with the number of new events since we last incremented it (which is 1) i.e. use ``statsd.increment``\n\n``update_stats`` would be appropriate IF we\u0027d sent more than one update since we last called ``update_stats``, or if we\u0027re counting a variable amount of \"happenings\" since the last call. For example, we use ``update_stats`` in proxy_logging to report \"N bytes have been sent since the last call to update_stats``.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"07965bf66580425f6ea22303313a824880be0d12","unresolved":false,"context_lines":[{"line_number":207,"context_line":"        if happen:"},{"line_number":208,"context_line":"            labels[\u0027event\u0027] \u003d \u0027deferred_skips\u0027"},{"line_number":209,"context_line":"            self.statsd.update_stats(\u0027swift_object_updater\u0027,"},{"line_number":210,"context_line":"                                     self.stats.skips,"},{"line_number":211,"context_line":"                                     labels\u003dlabels)"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"        # now drain the buckets..."}],"source_content_type":"text/x-python","patch_set":21,"id":"c5a162b9_8a1a6008","line":210,"range":{"start_line":210,"start_character":37,"end_line":210,"end_character":53},"in_reply_to":"f2fcde9d_d7341194","updated":"2025-11-04 15:34:02.000000000","message":"Done","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":224,"context_line":"                    # bucket has more deferred elements, re-insert in queue in"},{"line_number":225,"context_line":"                    # correct chronological position"},{"line_number":226,"context_line":"                    self.buckets_ordered_by_readiness.put(bucket)"},{"line_number":227,"context_line":"                labels \u003d self._get_acco_from_update(item)"},{"line_number":228,"context_line":"                self.stats.drains +\u003d 1"},{"line_number":229,"context_line":"                labels[\u0027event\u0027] \u003d \u0027deferred_drains\u0027"},{"line_number":230,"context_line":"                self.logger.increment(\"drains\")"}],"source_content_type":"text/x-python","patch_set":21,"id":"e9484ab4_6a1f8b21","line":227,"updated":"2025-10-28 19:17:42.000000000","message":"I wonder if your helper method might evolve to accept item and event and return a labels dict (``get_labels(item, event)``?) so that line 229 can be folded into here?\n\nsimilarly at line 240 and 245","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"07965bf66580425f6ea22303313a824880be0d12","unresolved":false,"context_lines":[{"line_number":224,"context_line":"                    # bucket has more deferred elements, re-insert in queue in"},{"line_number":225,"context_line":"                    # correct chronological position"},{"line_number":226,"context_line":"                    self.buckets_ordered_by_readiness.put(bucket)"},{"line_number":227,"context_line":"                labels \u003d self._get_acco_from_update(item)"},{"line_number":228,"context_line":"                self.stats.drains +\u003d 1"},{"line_number":229,"context_line":"                labels[\u0027event\u0027] \u003d \u0027deferred_drains\u0027"},{"line_number":230,"context_line":"                self.logger.increment(\"drains\")"}],"source_content_type":"text/x-python","patch_set":21,"id":"d575c80e_a4e53bb2","line":227,"in_reply_to":"e9484ab4_6a1f8b21","updated":"2025-11-04 15:34:02.000000000","message":"Acknowledged","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":729,"context_line":"        The iterator tries to clean up empty directories as it goes."},{"line_number":730,"context_line":"        \"\"\""},{"line_number":731,"context_line":"        # loop through async pending dirs for all policies"},{"line_number":732,"context_line":"        event \u003d \u0027\u0027"},{"line_number":733,"context_line":"        for asyncdir in self._listdir(device):"},{"line_number":734,"context_line":"            # we only care about directories"},{"line_number":735,"context_line":"            async_pending \u003d os.path.join(device, asyncdir)"}],"source_content_type":"text/x-python","patch_set":21,"id":"9d6311c0_9aac22ea","line":732,"updated":"2025-10-28 19:17:42.000000000","message":"+1 ``event`` ends up being the value used for ``labels[\u0027event\u0027]`` which is tottaly intuitive.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"07965bf66580425f6ea22303313a824880be0d12","unresolved":false,"context_lines":[{"line_number":729,"context_line":"        The iterator tries to clean up empty directories as it goes."},{"line_number":730,"context_line":"        \"\"\""},{"line_number":731,"context_line":"        # loop through async pending dirs for all policies"},{"line_number":732,"context_line":"        event \u003d \u0027\u0027"},{"line_number":733,"context_line":"        for asyncdir in self._listdir(device):"},{"line_number":734,"context_line":"            # we only care about directories"},{"line_number":735,"context_line":"            async_pending \u003d os.path.join(device, asyncdir)"}],"source_content_type":"text/x-python","patch_set":21,"id":"6943f4a0_4308e2ec","line":732,"in_reply_to":"9d6311c0_9aac22ea","updated":"2025-11-04 15:34:02.000000000","message":"Acknowledged","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":757,"context_line":"                for update_file in sorted(self._listdir(prefix_path),"},{"line_number":758,"context_line":"                                          reverse\u003dTrue):"},{"line_number":759,"context_line":"                    update_path \u003d os.path.join(prefix_path, update_file)"},{"line_number":760,"context_line":"                    labels \u003d {\u0027async pending directory\u0027: asyncdir}"},{"line_number":761,"context_line":"                    # GOOD LABELS?"},{"line_number":762,"context_line":"                    if not os.path.isfile(update_path):"},{"line_number":763,"context_line":"                        continue"}],"source_content_type":"text/x-python","patch_set":21,"id":"19ece1f5_a598c7f9","line":760,"updated":"2025-10-28 19:17:42.000000000","message":"label values must always have a constrained cardinality. There\u0027s an asyncdir for every device for every policy. That\u0027s going to be many thousands, which is likely too many.\n\nSo this is probably not a great label. But if I am wrong about the cardinality, then I\u0027d suggest that better labels might be the device and the policy (i.e. decompose the asyncdir).","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"07965bf66580425f6ea22303313a824880be0d12","unresolved":true,"context_lines":[{"line_number":757,"context_line":"                for update_file in sorted(self._listdir(prefix_path),"},{"line_number":758,"context_line":"                                          reverse\u003dTrue):"},{"line_number":759,"context_line":"                    update_path \u003d os.path.join(prefix_path, update_file)"},{"line_number":760,"context_line":"                    labels \u003d {\u0027async pending directory\u0027: asyncdir}"},{"line_number":761,"context_line":"                    # GOOD LABELS?"},{"line_number":762,"context_line":"                    if not os.path.isfile(update_path):"},{"line_number":763,"context_line":"                        continue"}],"source_content_type":"text/x-python","patch_set":21,"id":"c27e914f_e6467384","line":760,"in_reply_to":"19ece1f5_a598c7f9","updated":"2025-11-04 15:34:02.000000000","message":"Agree, I do not understand what labels would be appropriate here. I had placed a placeholder label as \u0027asyncdir\u0027 here. Open to discussions on good labels for errors where there isn\u0027t an account and a container!","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"63241e41356f6102445c1f249ddb7db02f738438","unresolved":true,"context_lines":[{"line_number":770,"context_line":"                        self.logger.error("},{"line_number":771,"context_line":"                            \u0027ERROR async pending file with unexpected \u0027"},{"line_number":772,"context_line":"                            \u0027name %s\u0027, update_path)"},{"line_number":773,"context_line":"                        continue"},{"line_number":774,"context_line":"                    # Async pendings are stored on disk like this:"},{"line_number":775,"context_line":"                    #"},{"line_number":776,"context_line":"                    # \u003cdevice\u003e/async_pending/\u003csuffix\u003e/\u003cobj_hash\u003e-\u003ctimestamp\u003e"}],"source_content_type":"text/x-python","patch_set":21,"id":"b512a41b_831e27bb","line":773,"updated":"2025-10-28 19:17:42.000000000","message":"you have defined ``labels`` and ``event`` but now the ``continue`` short circuits the loop and they are never used.","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"},{"author":{"_account_id":35790,"name":"Shreeya Deshpande","email":"shreeyad@nvidia.com","username":"shreeyad"},"change_message_id":"07965bf66580425f6ea22303313a824880be0d12","unresolved":false,"context_lines":[{"line_number":770,"context_line":"                        self.logger.error("},{"line_number":771,"context_line":"                            \u0027ERROR async pending file with unexpected \u0027"},{"line_number":772,"context_line":"                            \u0027name %s\u0027, update_path)"},{"line_number":773,"context_line":"                        continue"},{"line_number":774,"context_line":"                    # Async pendings are stored on disk like this:"},{"line_number":775,"context_line":"                    #"},{"line_number":776,"context_line":"                    # \u003cdevice\u003e/async_pending/\u003csuffix\u003e/\u003cobj_hash\u003e-\u003ctimestamp\u003e"}],"source_content_type":"text/x-python","patch_set":21,"id":"4178df7f_04e7a47e","line":773,"in_reply_to":"b512a41b_831e27bb","updated":"2025-11-04 15:34:02.000000000","message":"Acknowledged","commit_id":"de8af8fb4b221b70aa36fd2b695f570e9ef64c97"}]}