)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"02cdc3723049b7b87705653b2ba94d5e6bd3474e","unresolved":true,"context_lines":[{"line_number":7,"context_line":"expirer: Only try to delete containers if it might succeed"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Which is to say, if all the tasks we saw in the container were actually"},{"line_number":10,"context_line":"ready for processing (or the container was empty)."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I8c3f78d1d1850ab501180f884f81f84552617fb7"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"a4107c2a_0291e677","line":10,"updated":"2022-02-10 05:53:28.000000000","message":"More on why this is a good thing: Just *attempting* the delete will clear both info and listing shard ranges from cache: https://github.com/openstack/swift/blob/2.28.0/swift/proxy/controllers/container.py#L552-L555\n\nSo in a big cluster, you could have hundreds of expirers all  trying to list and delete containers that are known to still be draining. If your expiry containers get large enough to shard (protip: if you get big enough, they will!), that means they\u0027re basically always going the slow way, querying shards from the root before building up a proper listing.\n\nMeanwhile, the poor root\u0027s probably drowning trying to redirect updaters that want to clean up stale queue entries. Those guys *have* to get a redirect; the object server and the updater are the only ones that know the old queue time, and they don\u0027t have any handle to memcache.","commit_id":"3ebe8619019505397a5e0b8b5ce529b6313b1b25"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6e3a3d884cba0fc4b2788df349403f20dfe9c5de","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"2020c835_1de1a09e","updated":"2022-02-10 17:20:08.000000000","message":"mechanically this looks great!  seems like a good idea - nice optimization\n\nthere was never any principled reason we cleaned up containers the way the existing code was written - i think it was mostly KISS, but there\u0027s a strong advantage to avoid cache busting calls when your expirer containers are big busy and sharded.\n\nI couldn\u0027t quite follow the test churn.  It\u0027d be nice to see something demonstrating the cache issue that fails when the change is reverted; but even some positive and negative (test_does_delete, test_does_not_delete) would be great.","commit_id":"3ebe8619019505397a5e0b8b5ce529b6313b1b25"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"7f9c8f49b9cbe727fd3aaddfca74c4957e9eb7c4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"05599c5d_042e0ccd","updated":"2022-02-14 06:20:46.000000000","message":"Cool, so had a play with this today. Created a bunch of X-Delete-After objects, turned the expiring_objects_container_divisor down low so it\u0027ll use a bunch of expirer containers. \n\nThen ran the expirer one cycle at a time (from one node), and sure enough you see a bunch of objects expire, then next run their container is deleted. Not a bunch of container deletes all the time. So much more targetted.\n\nI think this is a big win. nice one Tim! ","commit_id":"229065d7810c29e5cda07667369969e347785561"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"12f00e0856a11243f00660f25d71b87643a418ce","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"0232aa7e_5b3b78ed","updated":"2022-02-11 05:59:31.000000000","message":"Having to wait for another cycle to confirm the container is empty is fine. And because it\u0027s checking pre-skipping based on process/processes divisor maths we actually have great confidence it is empty. I love this!\n\nI guess the only behavioural difference is the container deletes will be inlined with the expiry cycle (although a cycle later) rather then at the end, but I don\u0027t see that was a problem.\n\nI\u0027ll +2 because it seems to be well tested and looks great. Haven\u0027t tested in my SAIO though so would like another +2 before merge (or wait for me to SAIO test on my Monday).\n\nTime to go make dinner for the girls 😊 ","commit_id":"229065d7810c29e5cda07667369969e347785561"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"912935a58f9f5611a071f9ae4912e390ce868aee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"ec79692c_0781201a","updated":"2022-02-14 09:40:45.000000000","message":"In my previous comment I wrote\n\n  \"There\u0027s a string argument for being less eager ...\"\n\nTo avoid confusion, I meant to write:\n\n  \"There\u0027s a strong argument for being less eager ...\"\n\nThe patch does not change any function arguments, string or otherwise 😊","commit_id":"229065d7810c29e5cda07667369969e347785561"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"a30bc9055d652cdeada8fb14398fed29a08f931a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"f738e039_948f26e2","updated":"2022-02-26 00:40:31.000000000","message":"Results from prod look great; went from hundreds of container 409s per second to, like, one. Listing-shard-range cache-miss percentage went from \"usually in the 1-2% range with occasional spikes as high as 5%\" to \"under 0.5%, with only the occasional spike to 1-2%.\"","commit_id":"229065d7810c29e5cda07667369969e347785561"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"76afd1e965ecae5ea42b8128663dcd45dd954924","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"f1d9f4e7_67d42e0c","updated":"2022-02-11 10:05:57.000000000","message":"There\u0027s a string argument for being less eager to speculatively deleting containers, and the change is simple and expedient. LGTM but I haven\u0027t yet studied the unit tests in detail so only +1.","commit_id":"229065d7810c29e5cda07667369969e347785561"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"61df491a43a393ea91e8bac9cf5718d6d7a290a6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"5a990818_1195695e","updated":"2022-02-11 04:16:10.000000000","message":"recheck","commit_id":"229065d7810c29e5cda07667369969e347785561"}],"swift/obj/expirer.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6e3a3d884cba0fc4b2788df349403f20dfe9c5de","unresolved":true,"context_lines":[{"line_number":343,"context_line":"                     self.iter_task_containers_to_expire(task_account)]"},{"line_number":344,"context_line":""},{"line_number":345,"context_line":"                task_account_container_list_to_delete.extend("},{"line_number":346,"context_line":"                    task_account_container_list)"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":349,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":3,"id":"310e39ef_5253b86d","side":"PARENT","line":346,"updated":"2022-02-10 17:20:08.000000000","message":"oic, the problem here is that it was never conditional","commit_id":"8ef530d795df7f0300e761933c916546427a79fe"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7474862398c1e9662bcee4f13b78dfa7f5e85f67","unresolved":true,"context_lines":[{"line_number":299,"context_line":"                           target_account, target_container, target_object]),"},{"line_number":300,"context_line":"                       \u0027delete_timestamp\u0027: delete_timestamp,"},{"line_number":301,"context_line":"                       \u0027is_async_delete\u0027: is_async}"},{"line_number":302,"context_line":"            if container_empty or all_ready_to_expire:"},{"line_number":303,"context_line":"                self.containers_to_delete.add("},{"line_number":304,"context_line":"                    (task_account, task_container))"},{"line_number":305,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"f6c4188a_002f9df4","line":302,"updated":"2022-01-21 22:53:54.000000000","message":"Another strategy to consider: only try to delete containers that appear to be empty. We could even do it right here in iter_task_to_expire(). Way less tracking needs to be done, but it means the container will hang around for an extra cycle.","commit_id":"3ebe8619019505397a5e0b8b5ce529b6313b1b25"}],"test/unit/obj/test_expirer.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"76afd1e965ecae5ea42b8128663dcd45dd954924","unresolved":true,"context_lines":[{"line_number":145,"context_line":"        self.expirer \u003d expirer.ObjectExpirer(self.conf, logger\u003dself.logger,"},{"line_number":146,"context_line":"                                             swift\u003dself.fake_swift)"},{"line_number":147,"context_line":""},{"line_number":148,"context_line":"        # map of times to target object paths which should be expirerd now"},{"line_number":149,"context_line":"        self.expired_target_paths \u003d {"},{"line_number":150,"context_line":"            self.past_time: ["},{"line_number":151,"context_line":"                swob.wsgi_to_str(tgt) for tgt in ("}],"source_content_type":"text/x-python","patch_set":4,"id":"2bbdf75c_11abb1c9","line":148,"range":{"start_line":148,"start_character":62,"end_line":148,"end_character":70},"updated":"2022-02-11 10:05:57.000000000","message":"nit: expired","commit_id":"229065d7810c29e5cda07667369969e347785561"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"12f00e0856a11243f00660f25d71b87643a418ce","unresolved":true,"context_lines":[{"line_number":655,"context_line":"                list(self.expirer.iter_task_to_expire("},{"line_number":656,"context_line":"                    task_account_container_list, my_index, divisor)),"},{"line_number":657,"context_line":"                expected)"},{"line_number":658,"context_line":"        # not empty; not deleted"},{"line_number":659,"context_line":"        self.assertEqual(mock_delete_container.mock_calls, [])"},{"line_number":660,"context_line":""},{"line_number":661,"context_line":"        # the task queue has invalid task object"}],"source_content_type":"text/x-python","patch_set":4,"id":"524340ba_584ec245","line":658,"updated":"2022-02-11 05:59:31.000000000","message":"Not emptry in the sense that the flag `container_empty` is False because it had objects this cycle right. But by now it is empty.. oh I guess not because your only testing `iter_task_to_expire` so these don\u0027t actually get removed from the container, so yeah not empty 😊","commit_id":"229065d7810c29e5cda07667369969e347785561"}]}