)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"62e7e30cb55536f2659f81ca7b69a9989f04cbab","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Clay Gerrard \u003cclay.gerrard@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2024-04-03 08:57:39 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"expirer: randomize task_container iteration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: I8879dbd13527233c878dff764ec411ce9619ee39"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"51e43ce3_579f8f5e","line":7,"updated":"2024-04-03 16:18:52.000000000","message":"Could use some more explanation here -- I saw this because of a downstream ticket like \"speed up expirer startup\" and then it took me a bit to realize that this was *also* associated with the `delay_reaping` work, and the implications there.\n\nIn general, though, yeah, I like randomizing work order when/where we can.","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a3078f07376fca9c0e3b5ee4fe650c86505ff3b","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Clay Gerrard \u003cclay.gerrard@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2024-04-03 08:57:39 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"expirer: randomize task_container iteration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: I8879dbd13527233c878dff764ec411ce9619ee39"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"a8fde5fa_cf3f97ce","line":7,"in_reply_to":"51e43ce3_579f8f5e","updated":"2024-04-04 15:33:22.000000000","message":"agreed!  #willfix","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4c42adca18a8f29efb2fe75f9f9f742f02ddf0cf","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e4adc3ca_cbb217d2","updated":"2024-03-28 23:12:09.000000000","message":"tests still need work","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":36763,"name":"Anish Kachinthaya","display_name":"Anish","email":"akachinthaya@nvidia.com","username":"akachinthaya"},"change_message_id":"8005716bf58388ca90c0c4bcf05a7b43591c121e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"703111f5_bc78e6af","updated":"2024-04-02 19:35:30.000000000","message":"Made some comments mostly for small changes!","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a3078f07376fca9c0e3b5ee4fe650c86505ff3b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"25f6945e_4d9668bb","updated":"2024-04-04 15:33:22.000000000","message":"thanks for the feedback Tim \u0026 Anish!","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"6d1ba35f_dfd672a0","updated":"2024-04-03 14:10:20.000000000","message":"thanks for the feedback!","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"ddeef629_8180f45e","updated":"2024-04-05 14:26:35.000000000","message":"-1 because I can\u0027t get the tests to care about shuffling or not.\n\nThis makes sense when there is delay_reaping but when there isn\u0027t delay_reaping it seems to impact an implicit \"fairness\" - the objects that expired furthest in the past would get reaped first. That\u0027s moot when they are all one user\u0027s objects (that user will get their bytes quota back either way), but IIUC the task_account is not the user account, so we\u0027d be shuffling all users\u0027 tasks? \n\nSo a user who expired data days ago but got caught in some backlog may now wait longer for their bytes than a user who expired an hour ago (IIUC)? Of course, it\u0027s random so not that clear-cut. But I wonder if the shuffling should be configurable? or based off max(delay_reaping) - anything older than max(delay_reaping) is going to be reaped, so continue to do those in order??\n\nMore concrete issue: I removed all the shuffle mocks I could find and the tests still pass :/\n\n```\ndiff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py\nindex 5fbe3dfc4..172a23945 100644\n--- a/test/unit/obj/test_expirer.py\n+++ b/test/unit/obj/test_expirer.py\n@@ -835,8 +835,7 @@ class TestObjectExpirer(TestCase):\n             return captured_iter\n\n         with mock.patch(\u0027swift.obj.expirer.RateLimitedIterator\u0027,\n-                        side_effect\u003dfake_ratelimiter), \\\n-                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None):\n+                        side_effect\u003dfake_ratelimiter):\n             x.run_once()\n         self.assertEqual(calls, [([\n             self.make_task(self.past_time, target_path)\n@@ -1214,7 +1213,7 @@ class TestObjectExpirer(TestCase):\n                                      num_task_per_container, accounts)\n\n         # stub_shuffle returns results \"in order\"\n-        self._expirer_run_once_with_mocks(now\u003dnow, stub_shuffle\u003dlambda x: None)\n+        self._expirer_run_once_with_mocks(now\u003dnow)\n         iter_object_calls, delete_object_calls, num_iter_before_delete \u003d \\\n             self._sort_out_fake_swift_calls()\n         self.assertEqual((num_days_past_due * containers_per_day),\n@@ -1253,8 +1252,8 @@ class TestObjectExpirer(TestCase):\n         # round_robin_order will iter task_containers until *all* tasks are\n         # found before letting any work get done; so we should probably lower\n         # that cache size on startup\n-        # stub_max_objects_to_cache \u003d 10\n-        stub_max_objects_to_cache \u003d None\n+        stub_max_objects_to_cache \u003d 10\n+        # stub_max_objects_to_cache \u003d None\n         self._expirer_run_once_with_mocks(\n             now\u003dnow, stub_shuffle\u003dstub_shuffle,\n             stub_max_objects_to_cache\u003dstub_max_objects_to_cache)\n@@ -1301,7 +1300,6 @@ class TestObjectExpirer(TestCase):\n     def test_object_timestamp_break(self):\n         with mock.patch.object(self.expirer, \u0027delete_actual_object\u0027) \\\n                 as mock_method, \\\n-                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None), \\\n                 mock.patch.object(self.expirer, \u0027pop_queue\u0027):\n             self.expirer.run_once()\n\n@@ -1330,7 +1328,6 @@ class TestObjectExpirer(TestCase):\n         # all tasks are done\n         with mock.patch.object(self.expirer, \u0027delete_actual_object\u0027,\n                                lambda o, t, b: None), \\\n-                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None), \\\n                 mock.patch.object(self.expirer, \u0027pop_queue\u0027) as mock_method:\n             self.expirer.run_once()\n\n@@ -1386,7 +1383,6 @@ class TestObjectExpirer(TestCase):\n                                fail_delete_container), \\\n                 mock.patch.object(self.expirer, \u0027delete_actual_object\u0027,\n                                   fail_delete_actual_object), \\\n-                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None), \\\n                 mock.patch.object(self.expirer, \u0027pop_queue\u0027) as mock_pop:\n             self.expirer.run_once()\n             ```","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"e144fbb747c4dd993a2a2d52546ce5ec602f3572","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"cfb0adf8_8b81dd36","updated":"2024-04-05 11:43:02.000000000","message":"recheck\n\nunrelated failures in openstack.tests.functional.cloud.test_compute.TestCompute\n\n\"openstack.exceptions.ConflictException: ConflictException: 409: Client Error for url: https://213.32.74.51/compute/v2.1/servers, Multiple possible networks found, use a Network ID to be more specific.\"","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"d80205b1_40bb6360","updated":"2024-04-05 18:36:19.000000000","message":"sorry the tests are so confusing!  I\u0027m not sure this latest version helps.  In retrospect maybe I should have just started with 914714: sq: make test better | https://review.opendev.org/c/openstack/swift/+/914714 and let reviewers convince themselves that with enough tasks the randomization is still useful.\n\nI think there\u0027s some agreement about the suitability/usefulness of the change and also that the tests could use some additional polish before we merge.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"d6e6eb66_f5d7e661","updated":"2024-04-09 18:28:58.000000000","message":"I left some test fixups and suggestions stacked on the existing test follow up https://review.opendev.org/c/openstack/swift/+/915371","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"5e2aab72_705aeea7","updated":"2024-04-16 19:05:12.000000000","message":"I moved this more in the direction of configurable options so we can do more experiments without changing default behavior; I don\u0027t know if will have any material effects on the reservations about testing infra but I\u0027m satisfied enough with as far as i was able to get it in a direction I hope addresses the main concerns.","commit_id":"cfa0637615e117bb81de61be19ef725bca801ca5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d5fa76291e6279689212adc16ee7ff04bd0d3275","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"f9be9966_37b16a2e","updated":"2024-04-17 15:15:16.000000000","message":"thanks for looking at the new options!","commit_id":"ba64ac6c882c3ccb3709af218f0766813532b6a7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3857a860766e667ee312fa7cce0a3113c4c7ade7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9542782e_ff5b8edd","updated":"2024-04-19 14:24:13.000000000","message":"I now believe that we\u0027re going to have to change not just the \"order\" of task iteration but how we *distribute* task iteration in order to achive our scaling issues:\n\n916026: distributed parallel task container iteration | https://review.opendev.org/c/openstack/swift/+/916026\n\nI\u0027m not sure ths patch actually needs to be merged, but it might be better than what\u0027s on master.","commit_id":"0ace9d59ebb766fba1911eb515a45063e40780d2"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"f59e279ef60c8849f41eabf36d07794a33bd81be","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"102ffba9_df98808f","updated":"2024-05-17 04:44:26.000000000","message":"good direction on optimizing the long startup time caused by delay_reaping process. production code looks good to me, maybe we should set the default value of ``MAX_TASK_CONTAINER_TO_CACHE`` to ``2000``. will +1 after finish reviewing test code.","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"5c258447982a43fe0c412808b651b9d77dd5a218","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":14,"id":"c7192dfc_ac228d57","updated":"2024-05-17 05:45:59.000000000","message":"one crazy idea: instead of using ``delay_reaping`` implicitly, can we take it into consideration when proxy sets ``x-delete-at`` header?\n\nat proxy side: https://github.com/NVIDIA/swift/blob/master/swift/proxy/controllers/obj.py#L632,\nset header ``x-delete-at\u003d original_x-delete-at + delay_reaping_of_this_container``. proxy-server would need to lookup delay_reaping configs and use them.\n\nthen delay_reaping won\u0027t insert those not-ready-yet delete requests into the middle of delete task queue, only to the end. and we still need various new task container iteration strategies though, to keep improving the current task queue iteration.","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bafb02997778c1c94d74644c88a2a7d9efa6b03d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"2497ef47_0fbf84f6","in_reply_to":"c7192dfc_ac228d57","updated":"2024-05-17 15:12:13.000000000","message":"that has two side-effects that didn\u0027t meet our requirements when designing delay reaping\n\n#1 the object doesn\u0027t actually start to return 404 until right before it\u0027s reaped\n\n#2 a config change to reduce a container\u0027s delay_reaping wouldn\u0027t free up any bytes unless we \"scanned ahead\"\n\nWe could fix #1 by writing down the *real* x-delete-at in object metadata, and then \"just\" queuing the delete at in a \"reap-at\" task_container.  But we\u0027d still have to be willing to scan ahead on config change... so I agree: we still need various new task container iteration strategies though, to keep improving the current task queue iteration.\n\ni.e.\n\n918366: Parallel distirbuted task container iteration | https://review.opendev.org/c/openstack/swift/+/918366","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bafb02997778c1c94d74644c88a2a7d9efa6b03d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"2767bf79_0dd837c7","updated":"2024-05-17 15:12:13.000000000","message":"thanks Jian!","commit_id":"5ec63fb12beeb6446660bda9706c8907416fa8e4"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"64af11ce_f117cd65","updated":"2024-06-04 18:16:27.000000000","message":"this experiment seems to be running well in prod, but we still need the DNM legacy config shim as we haven\u0027t flopped over the `strategy \u003d parallel` yet","commit_id":"689ae8e8a1e7223ff20b44afa5c1790187751bfd"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f90b7c0499b839837e0cb54b1f445f09c284363c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":39,"id":"34939a37_e75cc72c","updated":"2024-10-28 12:21:32.000000000","message":"We should update (one of) the other duplicated config docs (sigh) and I suggest ripping out one of the other duplicates.\n\nhttps://review.opendev.org/c/openstack/swift/+/933561","commit_id":"8094e7d0b8f145a2ab2489560c7b03c6d4384ef5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ebf3086e9b8f17b50ee50a82adc68ee24c6af35d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":39,"id":"273b1186_c3696570","updated":"2024-10-28 19:53:16.000000000","message":"thanks Al!","commit_id":"8094e7d0b8f145a2ab2489560c7b03c6d4384ef5"}],"doc/manpages/object-expirer.conf.5":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"49026f3ba3cb5884795b3729d31271f6b649a47a","unresolved":true,"context_lines":[{"line_number":96,"context_line":"config option names should url-quote the paths.  The value is in seconds. The"},{"line_number":97,"context_line":"default is no delay for any tasks."},{"line_number":98,"context_line":".IP \\fBround_robin_task_cache_size\\fR"},{"line_number":99,"context_line":"Number of tasks objects to cache before processing."},{"line_number":100,"context_line":".IP \\fBnice_priority\\fR"},{"line_number":101,"context_line":"Modify scheduling priority of server processes. Niceness values range from -20"},{"line_number":102,"context_line":"(most favorable to the process) to 19 (least favorable to the process)."}],"source_content_type":"text/troff","patch_set":40,"id":"26d53204_f181df4c","line":99,"updated":"2024-10-29 08:39:43.000000000","message":"+1 oh yeah I forgot that we maintain config doc in *three* places !?!","commit_id":"798bc9759da318e7af0a9f5821af45cfc1ace198"}],"etc/object-expirer.conf-sample":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2669fa59477774972450d6ec4a73ac4df735f879","unresolved":true,"context_lines":[{"line_number":95,"context_line":"# When false each cycle the expirer will process all task containers oldest to"},{"line_number":96,"context_line":"# newest; N.B. this does not guarantee all tasks are processed oldest to"},{"line_number":97,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":98,"context_line":"# each days worth of expirations.  Randomized task queue processing may create"},{"line_number":99,"context_line":"# less hot spots."},{"line_number":100,"context_line":"# randomized_task_queue_iteration \u003d false"},{"line_number":101,"context_line":"#"},{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"92807780_b688f29a","line":99,"range":{"start_line":98,"start_character":35,"end_line":99,"end_character":17},"updated":"2024-04-17 14:25:47.000000000","message":"This implies that the default value of this option may result in hotspots, but I don\u0027t understand that to be true. IIUC the round-robin is designed to avoid hotspots and this option does not change that. \n\nOh, unless hotspot is referring to which task container is being listed? \n\nWhat this option does help with is start up delay, so worth mentioning that:\n\n```\n# When false each cycle the expirer will process all task containers oldest to\n# newest; N.B. this does not guarantee all tasks are processed oldest to\n# newest because of how tasks are randomly distributed into 100 containers for\n# each days worth of expirations. When using delay_reaping, processing task\n# containers in this order is likely to impose a significant delay after\n# restart before any DELETE requests are issued. This can be mitigated\n# by setting this option to true.\n```","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d5fa76291e6279689212adc16ee7ff04bd0d3275","unresolved":true,"context_lines":[{"line_number":95,"context_line":"# When false each cycle the expirer will process all task containers oldest to"},{"line_number":96,"context_line":"# newest; N.B. this does not guarantee all tasks are processed oldest to"},{"line_number":97,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":98,"context_line":"# each days worth of expirations.  Randomized task queue processing may create"},{"line_number":99,"context_line":"# less hot spots."},{"line_number":100,"context_line":"# randomized_task_queue_iteration \u003d false"},{"line_number":101,"context_line":"#"},{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"e20e08af_6602564c","line":99,"range":{"start_line":98,"start_character":35,"end_line":99,"end_character":17},"in_reply_to":"92807780_b688f29a","updated":"2024-04-17 15:15:16.000000000","message":"\u003e Oh, unless hotspot is referring to which task container is being listed?\n\nyeah, hot spotting the task-container listings; not the target_container updaters - as best I can tell that\u0027s about the only useful thing this does on it\u0027s own.\n\n\u003e What this option does help with is start up delay\n\nexcept it didn\u0027t help with that!  Because we still have to list 100s of containers to fill the round robin cache.","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"900188ecf05682c07cdf88ea68b77c13943632a7","unresolved":false,"context_lines":[{"line_number":95,"context_line":"# When false each cycle the expirer will process all task containers oldest to"},{"line_number":96,"context_line":"# newest; N.B. this does not guarantee all tasks are processed oldest to"},{"line_number":97,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":98,"context_line":"# each days worth of expirations.  Randomized task queue processing may create"},{"line_number":99,"context_line":"# less hot spots."},{"line_number":100,"context_line":"# randomized_task_queue_iteration \u003d false"},{"line_number":101,"context_line":"#"},{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"6474670a_c723d29a","line":99,"range":{"start_line":98,"start_character":35,"end_line":99,"end_character":17},"in_reply_to":"e20e08af_6602564c","updated":"2024-04-18 08:37:13.000000000","message":"Done","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"89a9bd0d6d2c59c477d713934bfe3430b8aa5337","unresolved":true,"context_lines":[{"line_number":97,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":98,"context_line":"# each days worth of expirations.  Randomized task queue processing may create"},{"line_number":99,"context_line":"# less hot spots."},{"line_number":100,"context_line":"# randomized_task_queue_iteration \u003d false"},{"line_number":101,"context_line":"#"},{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":103,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"ec30107e_11cc3ac0","line":100,"range":{"start_line":100,"start_character":2,"end_line":100,"end_character":33},"updated":"2024-04-17 14:35:19.000000000","message":"this is kind-of pedantic perhaps, but given how it\u0027s a pain to change a conf option once we\u0027ve rolled out to prod worth raising now...\n\nThis could be confused with the task caching business discussed in the next option ... \"We have this task cache that we round-robin by, so does randomized_task_queue_iteration negate the round-robin and make it random??\"\n\n``randomized_task_container_iteration`` would be calling it exactly what it is","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d5fa76291e6279689212adc16ee7ff04bd0d3275","unresolved":false,"context_lines":[{"line_number":97,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":98,"context_line":"# each days worth of expirations.  Randomized task queue processing may create"},{"line_number":99,"context_line":"# less hot spots."},{"line_number":100,"context_line":"# randomized_task_queue_iteration \u003d false"},{"line_number":101,"context_line":"#"},{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":103,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"6a6d64ed_5067fbd1","line":100,"range":{"start_line":100,"start_character":2,"end_line":100,"end_character":33},"in_reply_to":"ec30107e_11cc3ac0","updated":"2024-04-17 15:15:16.000000000","message":"that\u0027s fine by me, people know that the expirer queue is made up of task objects in task containers #willfix","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2669fa59477774972450d6ec4a73ac4df735f879","unresolved":true,"context_lines":[{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":103,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"},{"line_number":104,"context_line":"# to distribute DELETEs to multiple target containers."},{"line_number":105,"context_line":"# round_robin_task_cache_size \u003d 1000000"},{"line_number":106,"context_line":"#"},{"line_number":107,"context_line":"# recon_cache_path \u003d /var/cache/swift"},{"line_number":108,"context_line":"#"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"03b84148_b423ca93","line":105,"range":{"start_line":105,"start_character":32,"end_line":105,"end_character":39},"updated":"2024-04-17 14:25:47.000000000","message":"this is 10x the actual default 100000","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d5fa76291e6279689212adc16ee7ff04bd0d3275","unresolved":false,"context_lines":[{"line_number":102,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":103,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"},{"line_number":104,"context_line":"# to distribute DELETEs to multiple target containers."},{"line_number":105,"context_line":"# round_robin_task_cache_size \u003d 1000000"},{"line_number":106,"context_line":"#"},{"line_number":107,"context_line":"# recon_cache_path \u003d /var/cache/swift"},{"line_number":108,"context_line":"#"}],"source_content_type":"application/octet-stream","patch_set":9,"id":"c6ea3289_65dc0e97","line":105,"range":{"start_line":105,"start_character":32,"end_line":105,"end_character":39},"in_reply_to":"03b84148_b423ca93","updated":"2024-04-17 15:15:16.000000000","message":"heh, what ever it is it\u0027s too high by default ;)\n\n#willfix","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"f59e279ef60c8849f41eabf36d07794a33bd81be","unresolved":true,"context_lines":[{"line_number":94,"context_line":"# N.B. By default no delay_reaping value is configured for any accounts or"},{"line_number":95,"context_line":"# containers."},{"line_number":96,"context_line":"#"},{"line_number":97,"context_line":"# When false each cycle the expirer will process all task containers oldest to"},{"line_number":98,"context_line":"# newest; N.B. this does not guarantee all tasks are processed oldest to"},{"line_number":99,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":100,"context_line":"# each day\u0027s worth of expirations.  Randomized task container iteration may help"}],"source_content_type":"application/octet-stream","patch_set":14,"id":"51f6e392_04fe7b5e","line":97,"updated":"2024-05-17 04:44:26.000000000","message":"``process all task containers oldest to newest`` this is not necessary true as well, all task containers will be iterated by sorted order of timestamps, but within one day those 100 containers are just picked by random timestamps.","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bafb02997778c1c94d74644c88a2a7d9efa6b03d","unresolved":false,"context_lines":[{"line_number":94,"context_line":"# N.B. By default no delay_reaping value is configured for any accounts or"},{"line_number":95,"context_line":"# containers."},{"line_number":96,"context_line":"#"},{"line_number":97,"context_line":"# When false each cycle the expirer will process all task containers oldest to"},{"line_number":98,"context_line":"# newest; N.B. this does not guarantee all tasks are processed oldest to"},{"line_number":99,"context_line":"# newest because of how tasks are randomly distributed into 100 containers for"},{"line_number":100,"context_line":"# each day\u0027s worth of expirations.  Randomized task container iteration may help"}],"source_content_type":"application/octet-stream","patch_set":14,"id":"f9508b40_6e245e98","line":97,"in_reply_to":"51f6e392_04fe7b5e","updated":"2024-05-17 15:12:13.000000000","message":"The task containers are iterated in lexographical order because that\u0027s how the container-server will return the listing, at least until 2286 they will be processed \"in order\" \n\n\u003e N.B. this does not guarantee all tasks are processed oldest to newest because of how tasks are randomly distributed into 100 containers for each day\u0027s worth of expirations\n\n\u003e but within one day those 100 containers are just picked by random timestamps\n\nAre we trying to the say the same thing?\n\nI\u0027ll try and make the paragraph better.","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"f59e279ef60c8849f41eabf36d07794a33bd81be","unresolved":true,"context_lines":[{"line_number":102,"context_line":"# less hot spots listing task_containers from many processes."},{"line_number":103,"context_line":"# randomized_task_container_iteration \u003d false"},{"line_number":104,"context_line":"#"},{"line_number":105,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":106,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"},{"line_number":107,"context_line":"# to distribute DELETEs to multiple target containers."},{"line_number":108,"context_line":"# round_robin_task_cache_size \u003d 100000"}],"source_content_type":"application/octet-stream","patch_set":14,"id":"c65f758a_ad6c9dc3","line":105,"updated":"2024-05-17 04:44:26.000000000","message":"nit: s/tasks objects/task objects?","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bafb02997778c1c94d74644c88a2a7d9efa6b03d","unresolved":false,"context_lines":[{"line_number":102,"context_line":"# less hot spots listing task_containers from many processes."},{"line_number":103,"context_line":"# randomized_task_container_iteration \u003d false"},{"line_number":104,"context_line":"#"},{"line_number":105,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":106,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"},{"line_number":107,"context_line":"# to distribute DELETEs to multiple target containers."},{"line_number":108,"context_line":"# round_robin_task_cache_size \u003d 100000"}],"source_content_type":"application/octet-stream","patch_set":14,"id":"59d785f4_cd9ba04e","line":105,"in_reply_to":"c65f758a_ad6c9dc3","updated":"2024-05-17 15:12:13.000000000","message":"#willfix!  thank you!","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"f59e279ef60c8849f41eabf36d07794a33bd81be","unresolved":false,"context_lines":[{"line_number":105,"context_line":"# Number of tasks objects to cache before processing.  With many nodes it may"},{"line_number":106,"context_line":"# take some time to fill a larger cache_size but may also have a better chance"},{"line_number":107,"context_line":"# to distribute DELETEs to multiple target containers."},{"line_number":108,"context_line":"# round_robin_task_cache_size \u003d 100000"},{"line_number":109,"context_line":"#"},{"line_number":110,"context_line":"# recon_cache_path \u003d /var/cache/swift"},{"line_number":111,"context_line":"#"}],"source_content_type":"application/octet-stream","patch_set":14,"id":"cbe66aba_c65a4982","line":108,"updated":"2024-05-17 04:44:26.000000000","message":"I feel by adding this config itself and setting it to a lower value like 10000 should be a win already.","commit_id":"38e331570633496a7c6fdfa3139d5e89407e9d9b"}],"swift/obj/expirer.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4c42adca18a8f29efb2fe75f9f9f742f02ddf0cf","unresolved":true,"context_lines":[{"line_number":289,"context_line":"            task_container \u003d str(c[\u0027name\u0027])"},{"line_number":290,"context_line":"            timestamp \u003d self.delete_at_time_of_task_container(task_container)"},{"line_number":291,"context_line":"            if timestamp \u003e Timestamp.now():"},{"line_number":292,"context_line":"                break"},{"line_number":293,"context_line":"            container_list.append(task_container)"},{"line_number":294,"context_line":"            if len(container_list) \u003e MAX_TASK_CONTAINER_TO_CACHE:"},{"line_number":295,"context_line":"                shuffle(container_list)"}],"source_content_type":"text/x-python","patch_set":1,"id":"77b3fc21_7424e7a4","line":292,"updated":"2024-03-28 23:12:09.000000000","message":"I think most deployments are going to hit this before 100 days of backlog.","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"f59e279ef60c8849f41eabf36d07794a33bd81be","unresolved":true,"context_lines":[{"line_number":289,"context_line":"            task_container \u003d str(c[\u0027name\u0027])"},{"line_number":290,"context_line":"            timestamp \u003d self.delete_at_time_of_task_container(task_container)"},{"line_number":291,"context_line":"            if timestamp \u003e Timestamp.now():"},{"line_number":292,"context_line":"                break"},{"line_number":293,"context_line":"            container_list.append(task_container)"},{"line_number":294,"context_line":"            if len(container_list) \u003e MAX_TASK_CONTAINER_TO_CACHE:"},{"line_number":295,"context_line":"                shuffle(container_list)"}],"source_content_type":"text/x-python","patch_set":1,"id":"ab384448_e9bddeba","line":292,"in_reply_to":"77b3fc21_7424e7a4","updated":"2024-05-17 04:44:26.000000000","message":"is that how the default value came from?\n``MAX_TASK_CONTAINER_TO_CACHE \u003d 10000`` (100 * 100)\n\nsince random.shuffle won\u0027t work well beyond 2080 items (less permutations), see https://docs.python.org/3/library/random.html#random.shuffle. probably it\u0027s better to set the default value of MAX_TASK_CONTAINER_TO_CACHE as ``2000`` (20 days)?","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bafb02997778c1c94d74644c88a2a7d9efa6b03d","unresolved":false,"context_lines":[{"line_number":289,"context_line":"            task_container \u003d str(c[\u0027name\u0027])"},{"line_number":290,"context_line":"            timestamp \u003d self.delete_at_time_of_task_container(task_container)"},{"line_number":291,"context_line":"            if timestamp \u003e Timestamp.now():"},{"line_number":292,"context_line":"                break"},{"line_number":293,"context_line":"            container_list.append(task_container)"},{"line_number":294,"context_line":"            if len(container_list) \u003e MAX_TASK_CONTAINER_TO_CACHE:"},{"line_number":295,"context_line":"                shuffle(container_list)"}],"source_content_type":"text/x-python","patch_set":1,"id":"46455dec_06d36caf","line":292,"in_reply_to":"ab384448_e9bddeba","updated":"2024-05-17 15:12:13.000000000","message":"10K is the default return size for an account listing of containers:\n\nhttps://github.com/NVIDIA/swift/blob/master/swift/common/constraints.py#L37\n\nWe tend to have ~7-10 days of delay_reaping containers laying around; I imagine some deployments might use 30-90 days.  So, still all less than 10K.\n\n\u003e random.shuffle won\u0027t work well beyond 2080 items\n\nI\u0027d never noticed that confusing/thought-provoking note in the python docs before.  IME random.shuffle will work *quite* well on lists of \u003e2K items.\n\nhttps://en.wikipedia.org/wiki/Mersenne_Twister\n\n(2**19937 - 1) is an *enormous* number!!!  Quite enough to guarantee our little pool of a few thousand nodes are *very likely* be working on different containers to start.\n\nBut 10000! is an *inconcieveably huge* (like number of atoms in observable universe) sort of number.\n\nLuckily nothing about our assumptions for the behavior of random.shuffle make any requirement about all permutations of the list being equally likely.  Because ... well even enumerating all of them would take litterally forever.","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":36763,"name":"Anish Kachinthaya","display_name":"Anish","email":"akachinthaya@nvidia.com","username":"akachinthaya"},"change_message_id":"8005716bf58388ca90c0c4bcf05a7b43591c121e","unresolved":true,"context_lines":[{"line_number":405,"context_line":"                        \u0027container_count\u0027: container_count,"},{"line_number":406,"context_line":"                        \u0027obj_count\u0027: obj_count})"},{"line_number":407,"context_line":""},{"line_number":408,"context_line":"                task_account_container_iter \u003d ("},{"line_number":409,"context_line":"                    (task_account, task_container) for task_container in"},{"line_number":410,"context_line":"                    self.iter_task_containers_to_expire(task_account)"},{"line_number":411,"context_line":"                )"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":414,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":2,"id":"228550cc_1f856d41","line":411,"range":{"start_line":408,"start_character":21,"end_line":411,"end_character":17},"updated":"2024-04-02 19:35:30.000000000","message":"Just a possible approach but could possibly yield both (task_account, task_container) from `iter_task_containers_to_expire` so that you don\u0027t need to take this step and could just do\n```\ntask_account_container_iter \u003d self.iter_task_containers_to_expire(task_account)\n```\n\nYou might have had certain reasoning for taking this approach though, but since I see `iter_task_containers_to_expire` is not used elsewhere you could do this?","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":405,"context_line":"                        \u0027container_count\u0027: container_count,"},{"line_number":406,"context_line":"                        \u0027obj_count\u0027: obj_count})"},{"line_number":407,"context_line":""},{"line_number":408,"context_line":"                task_account_container_iter \u003d ("},{"line_number":409,"context_line":"                    (task_account, task_container) for task_container in"},{"line_number":410,"context_line":"                    self.iter_task_containers_to_expire(task_account)"},{"line_number":411,"context_line":"                )"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":414,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":2,"id":"4fba3239_8b4b0b3c","line":411,"range":{"start_line":408,"start_character":21,"end_line":411,"end_character":17},"in_reply_to":"228550cc_1f856d41","updated":"2024-04-03 14:10:20.000000000","message":"yeah i noticed that too.  Let me check if there\u0027s not a bunch of tests that expect iter_task_containers_to_expire to only yield containers; if not - I do agree it would improve maintainability to have the method actually yield out what the consumer wants rather than having to create a wrapper generator.","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":true,"context_lines":[{"line_number":287,"context_line":"            timestamp \u003d self.delete_at_time_of_task_container(task_container)"},{"line_number":288,"context_line":"            if timestamp \u003e Timestamp.now():"},{"line_number":289,"context_line":"                break"},{"line_number":290,"context_line":"            yield task_container"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":"    def get_grace_period(self, target_account, target_container):"},{"line_number":293,"context_line":"        return get_grace_period(self.grace_periods, target_account,"}],"source_content_type":"text/x-python","patch_set":4,"id":"654f72d8_75a36a7b","side":"PARENT","line":290,"updated":"2024-04-03 14:10:20.000000000","message":"It annoyed me that this was written as a generator but the caller compressed it to a list.  In an earlier revision of the test I changed the list comprehension to a generator comprehension but in review feedback it seemed like it\u0027d be better to just have this method yield out the type our consumer actually wants (a tuple of account/container)\n\nchanging the return type of this method didn\u0027t cause any test churn; it gets a lot of coverage through run_once (where it was converted to a list-of-tuples inline) but nothing was hitting it explicitly - which is fine (it\u0027s mostly an implementation detail HOW we iterate over the task-containers in the task-accountd); as long as we assert we list all the right ones - which many tests already do.","commit_id":"7bffb150fd2281e125cdb5ac99a627aa035a5ead"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":287,"context_line":"            timestamp \u003d self.delete_at_time_of_task_container(task_container)"},{"line_number":288,"context_line":"            if timestamp \u003e Timestamp.now():"},{"line_number":289,"context_line":"                break"},{"line_number":290,"context_line":"            yield task_container"},{"line_number":291,"context_line":""},{"line_number":292,"context_line":"    def get_grace_period(self, target_account, target_container):"},{"line_number":293,"context_line":"        return get_grace_period(self.grace_periods, target_account,"}],"source_content_type":"text/x-python","patch_set":4,"id":"95640094_04b0974f","side":"PARENT","line":290,"in_reply_to":"654f72d8_75a36a7b","updated":"2024-04-16 19:05:12.000000000","message":"Acknowledged","commit_id":"7bffb150fd2281e125cdb5ac99a627aa035a5ead"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":true,"context_lines":[{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":417,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":4,"id":"00ddc3e5_af2f75c1","line":414,"updated":"2024-04-03 14:10:20.000000000","message":"i tried passing iter_task_to_expire the task_account and having *it* call this method; but test_iter_task_to_expire really wanted to be able to test each of the containers built in setUp one at a time by controlling the list it passed in.\n\nI considered multiple alternatives\n\n1) use a mock to change this methods result\n2) use _setup_fake_swift to recreate just the containers each assertion wanted\n3) re-phrase all the assertions to catch each behavior in one pass\n\nIMHO they all created too much unrelated churn and little obvious value.  So I left it like this; which is probably marginally better than compressing the iter to a list before passing it in.","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":417,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":4,"id":"83aa2c2c_df6004a9","line":414,"in_reply_to":"00ddc3e5_af2f75c1","updated":"2024-04-05 14:26:35.000000000","message":"IMHO this is preferable to master","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"},{"line_number":415,"context_line":""},{"line_number":416,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":417,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":4,"id":"50d625b4_81a1a441","line":414,"in_reply_to":"83aa2c2c_df6004a9","updated":"2024-04-05 18:36:19.000000000","message":"Acknowledged","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"62e7e30cb55536f2659f81ca7b69a9989f04cbab","unresolved":true,"context_lines":[{"line_number":441,"context_line":"        :param kwargs: Extra keyword args to fulfill the Daemon interface; this"},{"line_number":442,"context_line":"                       daemon has no additional keyword args."},{"line_number":443,"context_line":"        \"\"\""},{"line_number":444,"context_line":"        sleep(random() * self.interval)"},{"line_number":445,"context_line":"        while True:"},{"line_number":446,"context_line":"            begin \u003d time()"},{"line_number":447,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"1613c6be_ed071539","line":444,"updated":"2024-04-03 16:18:52.000000000","message":"What kind of `interval` are we running with? I guess I\u0027d always assumed that most of the delay seen following a cluster-wide restart was due to this sleep right here...","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a3078f07376fca9c0e3b5ee4fe650c86505ff3b","unresolved":false,"context_lines":[{"line_number":441,"context_line":"        :param kwargs: Extra keyword args to fulfill the Daemon interface; this"},{"line_number":442,"context_line":"                       daemon has no additional keyword args."},{"line_number":443,"context_line":"        \"\"\""},{"line_number":444,"context_line":"        sleep(random() * self.interval)"},{"line_number":445,"context_line":"        while True:"},{"line_number":446,"context_line":"            begin \u003d time()"},{"line_number":447,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":4,"id":"31daaf9f_0c7a215f","line":444,"in_reply_to":"1613c6be_ed071539","updated":"2024-04-04 15:33:22.000000000","message":"we were running 1hr, we tried to reduce it to 30m - I also expected it to have a significant effect on the startup time - but we\u0027re seeing a window of 4hrs after a restart before DELETE requests start to show up.  Since the sleep is randomized, reducing the internval did not have a significant effect.","commit_id":"5c245ad040c91e5899080f729ddde3755c38583d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"from swift.container.reconciler import direct_delete_container_entry"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"MAX_TASK_CONTAINER_TO_CACHE \u003d 10000"},{"line_number":39,"context_line":"MAX_OBJECTS_TO_CACHE \u003d 100000"},{"line_number":40,"context_line":"ASYNC_DELETE_TYPE \u003d \u0027application/async-deleted\u0027"},{"line_number":41,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"f5f2e3bb_e3eedaf3","line":38,"updated":"2024-04-05 14:26:35.000000000","message":"let me check my understanding...expiry tasks are \"sharded\" into one of 100 containers per day per account, so this means that each batch will have up to 100 days worth of expiry task containers. It\u0027s therefore likely that on each restart the entire backlog of containers will be shuffled as one batch.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"from swift.container.reconciler import direct_delete_container_entry"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"MAX_TASK_CONTAINER_TO_CACHE \u003d 10000"},{"line_number":39,"context_line":"MAX_OBJECTS_TO_CACHE \u003d 100000"},{"line_number":40,"context_line":"ASYNC_DELETE_TYPE \u003d \u0027application/async-deleted\u0027"},{"line_number":41,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"8ff88e35_2672c315","line":38,"in_reply_to":"3d6aa478_21a64f71","updated":"2024-04-16 19:05:12.000000000","message":"Acknowledged","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":true,"context_lines":[{"line_number":35,"context_line":""},{"line_number":36,"context_line":"from swift.container.reconciler import direct_delete_container_entry"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"MAX_TASK_CONTAINER_TO_CACHE \u003d 10000"},{"line_number":39,"context_line":"MAX_OBJECTS_TO_CACHE \u003d 100000"},{"line_number":40,"context_line":"ASYNC_DELETE_TYPE \u003d \u0027application/async-deleted\u0027"},{"line_number":41,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3d6aa478_21a64f71","line":38,"in_reply_to":"f5f2e3bb_e3eedaf3","updated":"2024-04-05 18:36:19.000000000","message":"\u003e are \"sharded\" into one of 100 containers per day per account\n\nno, unless your using the the new \"generic task queue\" config there\u0027s only .expiring_objects and all tasks for all accounts go into containers in the legacy .expiring_objects account\n\n\u003e  this means that each batch will have up to 100 days worth of expiry task containers\n\nprobably less, generally.  But yes given 100 containers per day and 10K containers per page (one account listing page) you can have AT MAX 100 days of expiring tasks to evaluate.  Generally I expect you\u0027ll only ever have one-page from the account listing (at least with legacy .expiring_objects queue)\n\n\u003e It\u0027s therefore likely that on each restart the entire backlog of containers will be shuffled as one batch\n\nthat\u0027s true!  again; at least for single/legacy .expiring_objects account like we run; I\u0027m actually not entirely sure how the shuffling effects generic task queue style expiring task behavior; I doubt it\u0027s significantly incorrect tho.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":279,"context_line":""},{"line_number":280,"context_line":"    def iter_task_account_containers_to_expire(self, task_account):"},{"line_number":281,"context_line":"        \"\"\""},{"line_number":282,"context_line":"        Yields (task_acocunt, task_container) tuples under the task_account if"},{"line_number":283,"context_line":"        the delete at timestamp of task_container is past."},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"        N.B. This method \"batches\" the pages of the account listings so that it"}],"source_content_type":"text/x-python","patch_set":5,"id":"7ed68756_76551776","line":282,"updated":"2024-04-05 14:26:35.000000000","message":"s/task_acocunt/task_account/","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[{"line_number":279,"context_line":""},{"line_number":280,"context_line":"    def iter_task_account_containers_to_expire(self, task_account):"},{"line_number":281,"context_line":"        \"\"\""},{"line_number":282,"context_line":"        Yields (task_acocunt, task_container) tuples under the task_account if"},{"line_number":283,"context_line":"        the delete at timestamp of task_container is past."},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"        N.B. This method \"batches\" the pages of the account listings so that it"}],"source_content_type":"text/x-python","patch_set":5,"id":"bbdde650_05bb191e","line":282,"in_reply_to":"7ed68756_76551776","updated":"2024-04-05 18:36:19.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":283,"context_line":"        the delete at timestamp of task_container is past."},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"        N.B. This method \"batches\" the pages of the account listings so that it"},{"line_number":286,"context_line":"        can yeild the task_containers in a randomized order."},{"line_number":287,"context_line":"        \"\"\""},{"line_number":288,"context_line":"        container_list \u003d []"},{"line_number":289,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"90bde4d4_9afe6ee6","line":286,"range":{"start_line":286,"start_character":12,"end_line":286,"end_character":17},"updated":"2024-04-05 14:26:35.000000000","message":"s/yeild/yield/","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[{"line_number":283,"context_line":"        the delete at timestamp of task_container is past."},{"line_number":284,"context_line":""},{"line_number":285,"context_line":"        N.B. This method \"batches\" the pages of the account listings so that it"},{"line_number":286,"context_line":"        can yeild the task_containers in a randomized order."},{"line_number":287,"context_line":"        \"\"\""},{"line_number":288,"context_line":"        container_list \u003d []"},{"line_number":289,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1ec2e60d_4d4cef79","line":286,"range":{"start_line":286,"start_character":12,"end_line":286,"end_character":17},"in_reply_to":"90bde4d4_9afe6ee6","updated":"2024-04-05 18:36:19.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":408,"context_line":"                        \u0027container_count\u0027: container_count,"},{"line_number":409,"context_line":"                        \u0027obj_count\u0027: obj_count})"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"}],"source_content_type":"text/x-python","patch_set":5,"id":"e19a84ac_f8ac5a62","line":411,"range":{"start_line":411,"start_character":36,"end_line":411,"end_character":40},"updated":"2024-04-05 14:26:35.000000000","message":"nit: s/form/from/\n\nI make this typo *often*, along with \"repsonse\". I blame the clock skew between my left and right hand 😊.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[{"line_number":408,"context_line":"                        \u0027container_count\u0027: container_count,"},{"line_number":409,"context_line":"                        \u0027obj_count\u0027: obj_count})"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"}],"source_content_type":"text/x-python","patch_set":5,"id":"6b5c4002_82d31063","line":411,"range":{"start_line":411,"start_character":36,"end_line":411,"end_character":40},"in_reply_to":"e19a84ac_f8ac5a62","updated":"2024-04-05 18:36:19.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":409,"context_line":"                        \u0027obj_count\u0027: obj_count})"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"},{"line_number":415,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"f801cf1f_b62cda53","line":412,"updated":"2024-04-05 14:26:35.000000000","message":"I found *code* comment confusing until I grokked the history of *gerrit* comments, in particular I inferred \"this method\" to mean run_once.\n\nPerhaps (assuming I now understand):\n\n```\nFor backwards compatibility with tests, the task_account_container_iter\nis passed into iter_task_to_expire rather than constructed within\niter_task_to_expire.\n```","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[{"line_number":409,"context_line":"                        \u0027obj_count\u0027: obj_count})"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"                # the iter returned form this method is passed into"},{"line_number":412,"context_line":"                # iter_task_to_expire mainly for testing."},{"line_number":413,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":414,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"},{"line_number":415,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"91609b64_575f0490","line":412,"in_reply_to":"f801cf1f_b62cda53","updated":"2024-04-05 18:36:19.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"18c32a6a6e97877622d0c26f9b334fb65e40e726","unresolved":false,"context_lines":[{"line_number":352,"context_line":"                break"},{"line_number":353,"context_line":"            container_list.append(task_container)"},{"line_number":354,"context_line":"        if self.task_container_iteration_strategy \u003d\u003d \u0027randomized\u0027:"},{"line_number":355,"context_line":"            shuffle(container_list)"},{"line_number":356,"context_line":"        for task_container in container_list:"},{"line_number":357,"context_line":"            yield task_account, task_container"},{"line_number":358,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"b7b12e9c_ee9097df","line":355,"updated":"2024-06-14 01:27:10.000000000","message":"these two lines should be the bulk of this change....","commit_id":"193cffa6863064d8452351e314cbee69bdb9cd3f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"18c32a6a6e97877622d0c26f9b334fb65e40e726","unresolved":false,"context_lines":[{"line_number":354,"context_line":"        if self.task_container_iteration_strategy \u003d\u003d \u0027randomized\u0027:"},{"line_number":355,"context_line":"            shuffle(container_list)"},{"line_number":356,"context_line":"        for task_container in container_list:"},{"line_number":357,"context_line":"            yield task_account, task_container"},{"line_number":358,"context_line":""},{"line_number":359,"context_line":"    def get_delay_reaping(self, target_account, target_container):"},{"line_number":360,"context_line":"        return get_delay_reaping(self.delay_reaping_times, target_account,"}],"source_content_type":"text/x-python","patch_set":22,"id":"8ed59744_3cfe8934","line":357,"updated":"2024-06-14 01:27:10.000000000","message":"i think this refactor is just a drive-by; it\u0027s getting pulled out since this all changes in the follow-ups anyway.","commit_id":"193cffa6863064d8452351e314cbee69bdb9cd3f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"18c32a6a6e97877622d0c26f9b334fb65e40e726","unresolved":false,"context_lines":[{"line_number":366,"context_line":"        Yields task expire info dict which consists of task_account,"},{"line_number":367,"context_line":"        task_container, task_object, timestamp_to_delete, and target_path"},{"line_number":368,"context_line":"        \"\"\""},{"line_number":369,"context_line":"        for task_account, task_container in task_account_container_iter:"},{"line_number":370,"context_line":"            container_empty \u003d True"},{"line_number":371,"context_line":"            for o in self.swift.iter_objects(task_account, task_container):"},{"line_number":372,"context_line":"                container_empty \u003d False"}],"source_content_type":"text/x-python","patch_set":22,"id":"646f2cb2_3282098c","line":369,"updated":"2024-06-14 01:27:10.000000000","message":"more drive-by!","commit_id":"193cffa6863064d8452351e314cbee69bdb9cd3f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"18c32a6a6e97877622d0c26f9b334fb65e40e726","unresolved":false,"context_lines":[{"line_number":469,"context_line":"                # iter_task_to_expire rather than constructed within"},{"line_number":470,"context_line":"                # iter_task_to_expire."},{"line_number":471,"context_line":"                task_account_container_iter \u003d \\"},{"line_number":472,"context_line":"                    self.iter_task_account_containers_to_expire(task_account)"},{"line_number":473,"context_line":""},{"line_number":474,"context_line":"                # delete_task_iter is a generator to yield a dict of"},{"line_number":475,"context_line":"                # task_account, task_container, task_object, delete_timestamp,"}],"source_content_type":"text/x-python","patch_set":22,"id":"546967cb_9e798ffd","line":472,"updated":"2024-06-14 01:27:10.000000000","message":"more drive-by!","commit_id":"193cffa6863064d8452351e314cbee69bdb9cd3f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ebf3086e9b8f17b50ee50a82adc68ee24c6af35d","unresolved":false,"context_lines":[{"line_number":188,"context_line":"        # with lots of nodes and lots of tasks a large cache size can"},{"line_number":189,"context_line":"        # significantly delay processing; and caching tasks to round_robin"},{"line_number":190,"context_line":"        # target container order may be less necessary if there\u0027s only a few"},{"line_number":191,"context_line":"        # target containers or with randomized task container iteration"},{"line_number":192,"context_line":"        self.round_robin_task_cache_size \u003d int("},{"line_number":193,"context_line":"            conf.get(\u0027round_robin_task_cache_size\u0027, MAX_OBJECTS_TO_CACHE))"},{"line_number":194,"context_line":""}],"source_content_type":"text/x-python","patch_set":39,"id":"0003c95b_8935c521","line":191,"updated":"2024-10-28 19:53:16.000000000","message":"I think the goal is to get rid of \"randomized\" task container iteration - this comment is probably stale; we can probably just leave the descriptionin the example config as sufficient.","commit_id":"8094e7d0b8f145a2ab2489560c7b03c6d4384ef5"}],"test/unit/obj/test_expirer.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4c42adca18a8f29efb2fe75f9f9f742f02ddf0cf","unresolved":true,"context_lines":[{"line_number":1230,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1231,"context_line":""},{"line_number":1232,"context_line":"    def test_lucky_task_container_iteration(self):"},{"line_number":1233,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1234,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1235,"context_line":"        num_days_after_due \u003d 3"},{"line_number":1236,"context_line":"        containers_per_day \u003d 100"}],"source_content_type":"text/x-python","patch_set":1,"id":"bbffa002_e6c63d39","line":1233,"updated":"2024-03-28 23:12:09.000000000","message":"see 914714: sq: make test better | https://review.opendev.org/c/openstack/swift/+/914714 for a less realistic but more reasonable unittest","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1230,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1231,"context_line":""},{"line_number":1232,"context_line":"    def test_lucky_task_container_iteration(self):"},{"line_number":1233,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1234,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1235,"context_line":"        num_days_after_due \u003d 3"},{"line_number":1236,"context_line":"        containers_per_day \u003d 100"}],"source_content_type":"text/x-python","patch_set":1,"id":"7658fd64_fbf6ba20","line":1233,"in_reply_to":"bbffa002_e6c63d39","updated":"2024-04-16 19:05:12.000000000","message":"Acknowledged","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4c42adca18a8f29efb2fe75f9f9f742f02ddf0cf","unresolved":true,"context_lines":[{"line_number":1308,"context_line":"             for target_path in self.expired_target_paths[self.past_time]] +"},{"line_number":1309,"context_line":"            [mock.call(target_path, self.just_past_time, False)"},{"line_number":1310,"context_line":"             for target_path"},{"line_number":1311,"context_line":"             in self.expired_target_paths[self.just_past_time]])"},{"line_number":1312,"context_line":""},{"line_number":1313,"context_line":"    def test_failed_delete_keeps_entry(self):"},{"line_number":1314,"context_line":"        def deliberately_blow_up(actual_obj, timestamp):"}],"source_content_type":"text/x-python","patch_set":1,"id":"31dcdaed_1765fd0b","line":1311,"updated":"2024-03-28 23:12:09.000000000","message":"this test is order dependent too","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":1308,"context_line":"             for target_path in self.expired_target_paths[self.past_time]] +"},{"line_number":1309,"context_line":"            [mock.call(target_path, self.just_past_time, False)"},{"line_number":1310,"context_line":"             for target_path"},{"line_number":1311,"context_line":"             in self.expired_target_paths[self.just_past_time]])"},{"line_number":1312,"context_line":""},{"line_number":1313,"context_line":"    def test_failed_delete_keeps_entry(self):"},{"line_number":1314,"context_line":"        def deliberately_blow_up(actual_obj, timestamp):"}],"source_content_type":"text/x-python","patch_set":1,"id":"817810af_6a4c0ac3","line":1311,"in_reply_to":"31dcdaed_1765fd0b","updated":"2024-04-03 14:10:20.000000000","message":"Done","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4c42adca18a8f29efb2fe75f9f9f742f02ddf0cf","unresolved":true,"context_lines":[{"line_number":1338,"context_line":"            [mock.call(\u0027.expiring_objects\u0027, self.just_past_time,"},{"line_number":1339,"context_line":"             self.just_past_time + \u0027-\u0027 + target_path)"},{"line_number":1340,"context_line":"             for target_path"},{"line_number":1341,"context_line":"             in self.expired_target_paths[self.just_past_time]])"},{"line_number":1342,"context_line":""},{"line_number":1343,"context_line":"    def test_success_gets_counted(self):"},{"line_number":1344,"context_line":"        self.assertEqual(self.expirer.report_objects, 0)"}],"source_content_type":"text/x-python","patch_set":1,"id":"311503b2_4107d2e2","line":1341,"updated":"2024-03-28 23:12:09.000000000","message":"this test is order dependent too","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":1338,"context_line":"            [mock.call(\u0027.expiring_objects\u0027, self.just_past_time,"},{"line_number":1339,"context_line":"             self.just_past_time + \u0027-\u0027 + target_path)"},{"line_number":1340,"context_line":"             for target_path"},{"line_number":1341,"context_line":"             in self.expired_target_paths[self.just_past_time]])"},{"line_number":1342,"context_line":""},{"line_number":1343,"context_line":"    def test_success_gets_counted(self):"},{"line_number":1344,"context_line":"        self.assertEqual(self.expirer.report_objects, 0)"}],"source_content_type":"text/x-python","patch_set":1,"id":"fe2fcdf4_fbff6c64","line":1341,"in_reply_to":"311503b2_4107d2e2","updated":"2024-04-03 14:10:20.000000000","message":"Done","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"4c42adca18a8f29efb2fe75f9f9f742f02ddf0cf","unresolved":true,"context_lines":[{"line_number":1395,"context_line":"                \u0027.expiring_objects\u0027, self.just_past_time,"},{"line_number":1396,"context_line":"                self.just_past_time + \u0027-\u0027 + target_path)"},{"line_number":1397,"context_line":"            for target_path in self.expired_target_paths[self.just_past_time]"},{"line_number":1398,"context_line":"        ])"},{"line_number":1399,"context_line":"        self.assertEqual(self.expirer.logger.get_lines_for_level(\u0027info\u0027), ["},{"line_number":1400,"context_line":"            \u0027Pass beginning for task account .expiring_objects; \u0027"},{"line_number":1401,"context_line":"            \u00274 possible containers; 12 possible objects\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"88e42513_1581051e","line":1398,"updated":"2024-03-28 23:12:09.000000000","message":"this test is order dependent too","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":1395,"context_line":"                \u0027.expiring_objects\u0027, self.just_past_time,"},{"line_number":1396,"context_line":"                self.just_past_time + \u0027-\u0027 + target_path)"},{"line_number":1397,"context_line":"            for target_path in self.expired_target_paths[self.just_past_time]"},{"line_number":1398,"context_line":"        ])"},{"line_number":1399,"context_line":"        self.assertEqual(self.expirer.logger.get_lines_for_level(\u0027info\u0027), ["},{"line_number":1400,"context_line":"            \u0027Pass beginning for task account .expiring_objects; \u0027"},{"line_number":1401,"context_line":"            \u00274 possible containers; 12 possible objects\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"10e44971_f85df9dc","line":1398,"in_reply_to":"88e42513_1581051e","updated":"2024-04-03 14:10:20.000000000","message":"Done","commit_id":"0cf4fc9ee2ffb9cbc1b6061b497f6aa1d569d2e5"},{"author":{"_account_id":36763,"name":"Anish Kachinthaya","display_name":"Anish","email":"akachinthaya@nvidia.com","username":"akachinthaya"},"change_message_id":"8005716bf58388ca90c0c4bcf05a7b43591c121e","unresolved":true,"context_lines":[{"line_number":1197,"context_line":"                found_delete_object \u003d True"},{"line_number":1198,"context_line":"        return iter_object_calls, delete_object_calls, num_iter_before_delete"},{"line_number":1199,"context_line":""},{"line_number":1200,"context_line":"    def test_unlucky_task_container_iteration(self):"},{"line_number":1201,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1202,"context_line":"        num_days_after_due \u003d 3"},{"line_number":1203,"context_line":"        containers_per_day \u003d 10"}],"source_content_type":"text/x-python","patch_set":2,"id":"a92132a0_19ab92f9","line":1200,"range":{"start_line":1200,"start_character":8,"end_line":1200,"end_character":45},"updated":"2024-04-02 19:35:30.000000000","message":"Maybe rename to ordered since unlucky isn\u0027t completely clear, like `test_ordered_task_container_iteration`.","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":1197,"context_line":"                found_delete_object \u003d True"},{"line_number":1198,"context_line":"        return iter_object_calls, delete_object_calls, num_iter_before_delete"},{"line_number":1199,"context_line":""},{"line_number":1200,"context_line":"    def test_unlucky_task_container_iteration(self):"},{"line_number":1201,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1202,"context_line":"        num_days_after_due \u003d 3"},{"line_number":1203,"context_line":"        containers_per_day \u003d 10"}],"source_content_type":"text/x-python","patch_set":2,"id":"255be433_52bcc9fb","line":1200,"range":{"start_line":1200,"start_character":8,"end_line":1200,"end_character":45},"in_reply_to":"a92132a0_19ab92f9","updated":"2024-04-03 14:10:20.000000000","message":"Acknowledged","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":true,"context_lines":[{"line_number":1213,"context_line":"                                     num_days_after_due, containers_per_day,"},{"line_number":1214,"context_line":"                                     num_task_per_container, accounts)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"        # \"unlucky\" is defined as \"in order\""},{"line_number":1217,"context_line":"        self._expirer_run_once_with_mocks(now\u003dnow, stub_shuffle\u003dlambda x: None)"},{"line_number":1218,"context_line":"        iter_object_calls, delete_object_calls, num_iter_before_delete \u003d \\"},{"line_number":1219,"context_line":"            self._sort_out_fake_swift_calls()"}],"source_content_type":"text/x-python","patch_set":2,"id":"cabb9195_09571ca4","line":1216,"updated":"2024-04-03 14:10:20.000000000","message":"well, I tried to clear up how I\u0027m defining \"unlucky\" - the main behavioral assertion is trying to show that \"sometimes you can do a lot of listings before deletes\" - the fact that an \"ordered randomization\" (which is what we get currently) is the best way to demonstrate the worst possible \"randomization\" seemed like more of a \"test implementation detail\" as opposed to describing the behavior I was interested in.\n\nmaybe \"test_ordered_task_container_iteration_is_unlucky\" might be better!","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3a3078f07376fca9c0e3b5ee4fe650c86505ff3b","unresolved":false,"context_lines":[{"line_number":1213,"context_line":"                                     num_days_after_due, containers_per_day,"},{"line_number":1214,"context_line":"                                     num_task_per_container, accounts)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"        # \"unlucky\" is defined as \"in order\""},{"line_number":1217,"context_line":"        self._expirer_run_once_with_mocks(now\u003dnow, stub_shuffle\u003dlambda x: None)"},{"line_number":1218,"context_line":"        iter_object_calls, delete_object_calls, num_iter_before_delete \u003d \\"},{"line_number":1219,"context_line":"            self._sort_out_fake_swift_calls()"}],"source_content_type":"text/x-python","patch_set":2,"id":"c452aef3_f6582baa","line":1216,"in_reply_to":"cabb9195_09571ca4","updated":"2024-04-04 15:33:22.000000000","message":"Done","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":36763,"name":"Anish Kachinthaya","display_name":"Anish","email":"akachinthaya@nvidia.com","username":"akachinthaya"},"change_message_id":"8005716bf58388ca90c0c4bcf05a7b43591c121e","unresolved":true,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_lucky_task_container_iteration(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":2,"id":"7adf3fce_e45785b2","line":1233,"range":{"start_line":1233,"start_character":8,"end_line":1233,"end_character":43},"updated":"2024-04-02 19:35:30.000000000","message":"Same with renaming - maybe to shuffled instead of lucky, like `test_shuffled_task_container_iteration`.","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_lucky_task_container_iteration(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":2,"id":"d7a2d2f7_c540f738","line":1233,"range":{"start_line":1233,"start_character":8,"end_line":1233,"end_character":43},"in_reply_to":"7adf3fce_e45785b2","updated":"2024-04-03 14:10:20.000000000","message":"Acknowledged","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":36763,"name":"Anish Kachinthaya","display_name":"Anish","email":"akachinthaya@nvidia.com","username":"akachinthaya"},"change_message_id":"8005716bf58388ca90c0c4bcf05a7b43591c121e","unresolved":true,"context_lines":[{"line_number":1270,"context_line":"        self.assertEqual("},{"line_number":1271,"context_line":"            non_grace_task_per_container * containers_per_day,"},{"line_number":1272,"context_line":"            len(delete_object_calls))"},{"line_number":1273,"context_line":"        # with a sufficiently few task_containers this might randomly fail"},{"line_number":1274,"context_line":"        self.assertLess(num_iter_before_delete,"},{"line_number":1275,"context_line":"                        num_days_past_due * containers_per_day)"},{"line_number":1276,"context_line":""},{"line_number":1277,"context_line":"    def test_run_once_unicode_problem(self):"},{"line_number":1278,"context_line":"        requests \u003d []"},{"line_number":1279,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1b1a720c_a06ad771","line":1276,"range":{"start_line":1273,"start_character":0,"end_line":1276,"end_character":0},"updated":"2024-04-02 19:35:30.000000000","message":"Is there a way to deterministically test this maybe by reversing the order instead of shuffling? Not sure if this is required","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"49302cf62834ea6b91562ebdbe406f7fbe9ff62e","unresolved":false,"context_lines":[{"line_number":1270,"context_line":"        self.assertEqual("},{"line_number":1271,"context_line":"            non_grace_task_per_container * containers_per_day,"},{"line_number":1272,"context_line":"            len(delete_object_calls))"},{"line_number":1273,"context_line":"        # with a sufficiently few task_containers this might randomly fail"},{"line_number":1274,"context_line":"        self.assertLess(num_iter_before_delete,"},{"line_number":1275,"context_line":"                        num_days_past_due * containers_per_day)"},{"line_number":1276,"context_line":""},{"line_number":1277,"context_line":"    def test_run_once_unicode_problem(self):"},{"line_number":1278,"context_line":"        requests \u003d []"},{"line_number":1279,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fda40bb_d69f4589","line":1276,"range":{"start_line":1273,"start_character":0,"end_line":1276,"end_character":0},"in_reply_to":"1b1a720c_a06ad771","updated":"2024-04-03 14:10:20.000000000","message":"heh, yeah this test was useful (to me) when convincing myself the shuffle actually \"works\" in \"realistic\" scenarios - but the requirements of round_robin_order to produce 100K ready tasks to avoid doing all the listings before ANY deletes makes this test take WAY to long.\n\nWe should probably squash my follow up before we actually merge:\n\nhttps://review.opendev.org/c/openstack/swift/+/914714/2/test/unit/obj/test_expirer.py#1250\n\nBut I left this test as I developed it in-case anyone wanted to play with this big numbers and see for themselves how my changes would behave against a less-contrived and more production-like scenario.","commit_id":"e4e5917364a1266b7fad3adcb7c24b5c8ea06fd7"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":836,"context_line":""},{"line_number":837,"context_line":"        with mock.patch(\u0027swift.obj.expirer.RateLimitedIterator\u0027,"},{"line_number":838,"context_line":"                        side_effect\u003dfake_ratelimiter), \\"},{"line_number":839,"context_line":"                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None):"},{"line_number":840,"context_line":"            x.run_once()"},{"line_number":841,"context_line":"        self.assertEqual(calls, [(["},{"line_number":842,"context_line":"            self.make_task(self.past_time, target_path)"}],"source_content_type":"text/x-python","patch_set":5,"id":"e8878dc2_3d380f38","line":839,"range":{"start_line":839,"start_character":54,"end_line":839,"end_character":70},"updated":"2024-04-05 14:26:35.000000000","message":"not sure this lambda is needed? but in fact I can remove the mock entirely and the test passes.\n\nI think it is because dump_obj_cache_in_round_robin sorts the tasks by a/c, and the test setUp conveniently arranges for all tasks to have unique ascending a/c, so regardless of the order in which the containers are yielded, the tasks will sort the same","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":836,"context_line":""},{"line_number":837,"context_line":"        with mock.patch(\u0027swift.obj.expirer.RateLimitedIterator\u0027,"},{"line_number":838,"context_line":"                        side_effect\u003dfake_ratelimiter), \\"},{"line_number":839,"context_line":"                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None):"},{"line_number":840,"context_line":"            x.run_once()"},{"line_number":841,"context_line":"        self.assertEqual(calls, [(["},{"line_number":842,"context_line":"            self.make_task(self.past_time, target_path)"}],"source_content_type":"text/x-python","patch_set":5,"id":"1d2a6594_188c89e5","line":839,"range":{"start_line":839,"start_character":54,"end_line":839,"end_character":70},"in_reply_to":"53d9ebcf_dfbb9879","updated":"2024-04-16 19:05:12.000000000","message":"\u003e also might hide an unintended change\n\nI\u0027m not sure my imagination about \"hiding an unintended change\" is as good as yours here.  But we both seem to agree it\u0027s not absolutely necessary to mock shuffle in this test with the current setup/patching so it seems like a reasonable compromise to just comment we\u0027re introducing a foot-gun and leaving it for future us to deal with.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":true,"context_lines":[{"line_number":836,"context_line":""},{"line_number":837,"context_line":"        with mock.patch(\u0027swift.obj.expirer.RateLimitedIterator\u0027,"},{"line_number":838,"context_line":"                        side_effect\u003dfake_ratelimiter), \\"},{"line_number":839,"context_line":"                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None):"},{"line_number":840,"context_line":"            x.run_once()"},{"line_number":841,"context_line":"        self.assertEqual(calls, [(["},{"line_number":842,"context_line":"            self.make_task(self.past_time, target_path)"}],"source_content_type":"text/x-python","patch_set":5,"id":"f3559c53_a97df65f","line":839,"range":{"start_line":839,"start_character":54,"end_line":839,"end_character":70},"in_reply_to":"e8878dc2_3d380f38","updated":"2024-04-05 18:36:19.000000000","message":"I agree, the reason the test passes reliably without this \"fix\" is in fact because of the batching in the round_robin_order iter.  But the assertion below is *very* opionated about the order of the captured calls.\n\nI discovered this test is opionated about the order (and struggles with the container-listing shuffle) when I removed the batching/round_robin_order in a wip patch:\n\n914730: wip: use bucketized skipping in expirer | https://review.opendev.org/c/openstack/swift/+/914730\n\nDo you think it\u0027s better to leave out this test \"fix\" until it\u0027s absolutely required?  Or better to address the \"potential instability\" in the change that introduces the shuffle (since this test passes currently, as long as you mock shuffle) even if you *remove* the round_robin_order iter who\u0027s batching (currently) \"fixes\" the shuffle?","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":836,"context_line":""},{"line_number":837,"context_line":"        with mock.patch(\u0027swift.obj.expirer.RateLimitedIterator\u0027,"},{"line_number":838,"context_line":"                        side_effect\u003dfake_ratelimiter), \\"},{"line_number":839,"context_line":"                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None):"},{"line_number":840,"context_line":"            x.run_once()"},{"line_number":841,"context_line":"        self.assertEqual(calls, [(["},{"line_number":842,"context_line":"            self.make_task(self.past_time, target_path)"}],"source_content_type":"text/x-python","patch_set":5,"id":"53d9ebcf_dfbb9879","line":839,"range":{"start_line":839,"start_character":54,"end_line":839,"end_character":70},"in_reply_to":"f3559c53_a97df65f","updated":"2024-04-09 18:28:58.000000000","message":"hmmm, good point. I\u0027m in two minds - it\u0027s useful to avoid future test breakage, but also might hide an unintended change. How about swapping the mock for a comment \n\n```\nThe relatively small number of tasks used by this test makes container\niter shuffling irrelevant. If that ever changes then shuffle may need\nto be mocked with a no-op.```\n\n...there may be a better form of words.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":1104,"context_line":"        tasks for accounts[0] is in the queue."},{"line_number":1105,"context_line":""},{"line_number":1106,"context_line":"        :param now: the timestamp used consistently as now by the expirer"},{"line_number":1107,"context_line":"        :param num_days_past_due: number of task_containers days \u003c\u003d now"},{"line_number":1108,"context_line":"        :param containers_per_day: common.utils.get_expirer_container creates"},{"line_number":1109,"context_line":"                                   at most 100 containers per day by default"},{"line_number":1110,"context_line":"        :param num_tasks_per_container: the number of queue entries per"}],"source_content_type":"text/x-python","patch_set":5,"id":"cf50cf73_aea04554","line":1107,"updated":"2024-04-05 14:26:35.000000000","message":"add num_days_after_due","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":false,"context_lines":[{"line_number":1104,"context_line":"        tasks for accounts[0] is in the queue."},{"line_number":1105,"context_line":""},{"line_number":1106,"context_line":"        :param now: the timestamp used consistently as now by the expirer"},{"line_number":1107,"context_line":"        :param num_days_past_due: number of task_containers days \u003c\u003d now"},{"line_number":1108,"context_line":"        :param containers_per_day: common.utils.get_expirer_container creates"},{"line_number":1109,"context_line":"                                   at most 100 containers per day by default"},{"line_number":1110,"context_line":"        :param num_tasks_per_container: the number of queue entries per"}],"source_content_type":"text/x-python","patch_set":5,"id":"b42e017d_646df918","line":1107,"in_reply_to":"cf50cf73_aea04554","updated":"2024-04-05 18:36:19.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":1214,"context_line":"                                     num_task_per_container, accounts)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"        # stub_shuffle returns results \"in order\""},{"line_number":1217,"context_line":"        self._expirer_run_once_with_mocks(now\u003dnow, stub_shuffle\u003dlambda x: None)"},{"line_number":1218,"context_line":"        iter_object_calls, delete_object_calls, num_iter_before_delete \u003d \\"},{"line_number":1219,"context_line":"            self._sort_out_fake_swift_calls()"},{"line_number":1220,"context_line":"        self.assertEqual((num_days_past_due * containers_per_day),"}],"source_content_type":"text/x-python","patch_set":5,"id":"4c962a2a_3e44e78b","line":1217,"range":{"start_line":1217,"start_character":49,"end_line":1217,"end_character":78},"updated":"2024-04-05 14:26:35.000000000","message":"test passes without this","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":true,"context_lines":[{"line_number":1214,"context_line":"                                     num_task_per_container, accounts)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"        # stub_shuffle returns results \"in order\""},{"line_number":1217,"context_line":"        self._expirer_run_once_with_mocks(now\u003dnow, stub_shuffle\u003dlambda x: None)"},{"line_number":1218,"context_line":"        iter_object_calls, delete_object_calls, num_iter_before_delete \u003d \\"},{"line_number":1219,"context_line":"            self._sort_out_fake_swift_calls()"},{"line_number":1220,"context_line":"        self.assertEqual((num_days_past_due * containers_per_day),"}],"source_content_type":"text/x-python","patch_set":5,"id":"6b2b418b_a19ed6dc","line":1217,"range":{"start_line":1217,"start_character":49,"end_line":1217,"end_character":78},"in_reply_to":"4c962a2a_3e44e78b","updated":"2024-04-05 18:36:19.000000000","message":"gah, you\u0027re right.  This test isn\u0027t passing because we don\u0027t shuffle - it\u0027s passing because there\u0027s not enough tasks to fill the round_robin_order queue. 😭\n\nIf I add `stub_max_objects_to_cache\u003d59` it fails pretty reliably for me when i remove the mocked shuffle.\n\nI\u0027m sure you can tell from the differences in cardinality from the negative test and happy path and my follow-on work:\n\n914714: sq: make test better | https://review.opendev.org/c/openstack/swift/+/914714\n\n... that I wasn\u0027t really sure how best to trade off with realism and practicality when demonstrating the behavior of task_container randomization (and it\u0027s interaction with round_robin_order).\n\nPerhaps I should start with both tests in *this* change having a signficant enough backlog to \"break out\" of the round_robin_order caching and then mock out w/ stub_max_objects_to_cache so I can reduce cardinality of BOTH tests in the follow up (which we can probably squash before merge; or now if it\u0027s just not useful to try and demonstrate the realistic effect of shuffle in a unittest).","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1214,"context_line":"                                     num_task_per_container, accounts)"},{"line_number":1215,"context_line":""},{"line_number":1216,"context_line":"        # stub_shuffle returns results \"in order\""},{"line_number":1217,"context_line":"        self._expirer_run_once_with_mocks(now\u003dnow, stub_shuffle\u003dlambda x: None)"},{"line_number":1218,"context_line":"        iter_object_calls, delete_object_calls, num_iter_before_delete \u003d \\"},{"line_number":1219,"context_line":"            self._sort_out_fake_swift_calls()"},{"line_number":1220,"context_line":"        self.assertEqual((num_days_past_due * containers_per_day),"}],"source_content_type":"text/x-python","patch_set":5,"id":"c968fa04_acd2354d","line":1217,"range":{"start_line":1217,"start_character":49,"end_line":1217,"end_character":78},"in_reply_to":"6b2b418b_a19ed6dc","updated":"2024-04-16 19:05:12.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_randomized_task_container_iteration_hastens_deletes(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":5,"id":"20424778_e1156745","line":1233,"updated":"2024-04-05 14:26:35.000000000","message":"if the goal of these two tests is to demonstrate the benefit of shuffle vs no-shuffle then I\u0027d prefer to see a parameterised test helper with the only variable being the shuffle, and contrasting assertions on the outcome. As it is the no-shuffle test isn\u0027t fairly compared to this case because it cannot yield any work until all the tasks have been iterated over due to the 100000 sized object cache, whereas this test yields \u003e 100000 objects. \n\nIf I increase the test dimensions for no shuffle to match this test then it fails with ``num_iter_before_delete \u003c num_days_past_due * containers_per_day``, but num_iter_before_delete is \u003e than with shuffling, as expected.\n\nNone of which makes me doubt the efficacy of the shuffling, just the tests.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":true,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_randomized_task_container_iteration_hastens_deletes(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":5,"id":"864019c6_e4a8b899","line":1233,"in_reply_to":"20424778_e1156745","updated":"2024-04-05 18:36:19.000000000","message":"\u003e I\u0027d prefer to see a parameterised test helper with the only variable being the shuffle\n\nI think I read this originally as suggesting these tests are too DAMP, and I think we (sometimes) have a slightly difference in preference for how much we should DRY out tests (in general).  These tests are just 30-40 lines long; I tried to \"parameterize\" the test helpers that were about setup/execution already.\n\nI think in retrospect you were mainly trying to highlight the test-bug caused by the difference in paramertized setup of the negative test would have been avoided if both tests used the same setup cardinality (which is quite reasonable feedback for this particular pair of negative/happy-path tests, with no particular implications for test-infra-design - I think I only diviated from mirroring the setup vars while trying to avoid (unsuccessfully) writing  TWO super long-running tests that need to be fixed before merge).  But, as an experiment I\u0027ve tried to see what it looks like to add some default kwarg values to _setup_a_bunch_of_tasks and I\u0027m not entirely unhappy with the result.  I\u0027ll update it to see if you like it better, and in the follow-on \"sq\" patch I\u0027ll update the defaults to reduce cardinality/runtime of BOTH tests by using stub_max_objects_to_cache.\n\nThanks for spotting the bug in the negative test!\n\n\u003e If I increase the test dimensions for no shuffle to match this test then it fails [...], as expected\n\nThis was a helpful/useful observation, thank you.  I TOO had to learn how much round_robin_order steps on the toes of the \"effectiveness\" of the shuffle (although I mostly spent my time in the happy-path test-case).  I\u0027m glad that it was easy for you to adjust the setup in the negative test by twiddling a few top level vars and mostly prefer test infra that encourages that kind of playing without breaking any other tests.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_randomized_task_container_iteration_hastens_deletes(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":5,"id":"29367cbf_0b24b303","line":1233,"in_reply_to":"60fc45fc_ec4fba18","updated":"2024-06-04 18:16:27.000000000","message":"Acknowledged","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":true,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_randomized_task_container_iteration_hastens_deletes(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":5,"id":"60fc45fc_ec4fba18","line":1233,"in_reply_to":"6f8a1ebd_9bf309ba","updated":"2024-04-16 19:05:12.000000000","message":"\u003e The only value of the unshuffled test is to show that shuffling helps\n\nI think that\u0027s an unfairly single variable way to evaluate the \"value\" of the test - in the context of THIS change, during review, right NOW - maybe that value significantly outweights any other potential/theoretical value; but IME once this change merges it will live on for years in the swift code base and the value prop changes.\n\nIn the future it may come to document why sometimes ops observes slower startup on expirer than others: i.e. \"in order delays delete\" :lightbulb:.  If the future it may come to form the basis of another test that needs to demonstrate a different kind of worse case expirer backlog: i.e.  \"task container iteration delay\".  As independent tests they establish a pattern for using the new test-infra to setup a queue, run the expirer, and evaluate the ratio and order of listings - should we want to change task iteration again later it will be useful to have a pattern to consider worst/common/best case scenarios.\n\n\u003e if the goal of these two tests is to demonstrate the benefit of shuffle vs no-shuffle\n\u003e their usefulness relies on them being exactly the same except for the stub_shuffle and the final assertion.\n\nI don\u0027t think I misunderstood what you were originally advocating, it wasn\u0027t an issue of not clearly communicating your preference.  Certainly the test bug in the earlier revision doesn\u0027t really help my case - but I think I said even then we might disgree about the optimum trade off between shorter/more-similar tests being easier to review and the long term value/cost for maintainability.  I could be wrong about the \"best\" way to write to these tests; but I\u0027m hoping we can find a compromise.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1230,"context_line":"        self.assertEqual(num_iter_before_delete,"},{"line_number":1231,"context_line":"                         num_days_past_due * containers_per_day)"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def test_randomized_task_container_iteration_hastens_deletes(self):"},{"line_number":1234,"context_line":"        # these products get pretty big; this test takes \u003e1m OOM"},{"line_number":1235,"context_line":"        num_days_past_due \u003d 7"},{"line_number":1236,"context_line":"        num_days_after_due \u003d 3"}],"source_content_type":"text/x-python","patch_set":5,"id":"6f8a1ebd_9bf309ba","line":1233,"in_reply_to":"864019c6_e4a8b899","updated":"2024-04-09 18:28:58.000000000","message":"The suggestion to share a helper method wasn\u0027t about DRYing out. As I understand it, the tests are demonstrating the efficacy of shuffling. The only value of the unshuffled test is to show that shuffling helps - it is not adding any unit test coverage and is in fact testing deliberately regressed code.\n\nThe two tests are a lot shorter in the latest patchset, so it\u0027s easier to visually inspect their similarity, but I\u0027d still advocate that their usefulness relies on them being *exactly* the same except for the stub_shuffle and the final assertion.","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"d896c001014c7d01daae7818c68f4db441a99de5","unresolved":true,"context_lines":[{"line_number":1253,"context_line":"        # round_robin_order will iter task_containers until *all* tasks are"},{"line_number":1254,"context_line":"        # found before letting any work get done; so we should probably lower"},{"line_number":1255,"context_line":"        # that cache size on startup"},{"line_number":1256,"context_line":"        # stub_max_objects_to_cache \u003d 10"},{"line_number":1257,"context_line":"        stub_max_objects_to_cache \u003d None"},{"line_number":1258,"context_line":"        self._expirer_run_once_with_mocks("},{"line_number":1259,"context_line":"            now\u003dnow, stub_shuffle\u003dstub_shuffle,"}],"source_content_type":"text/x-python","patch_set":5,"id":"b42132e2_44935443","line":1256,"updated":"2024-04-05 14:26:35.000000000","message":"is this meant to be commented? looks like this is the only place stub_max_objects_to_cache arg is used and it\u0027s None","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1253,"context_line":"        # round_robin_order will iter task_containers until *all* tasks are"},{"line_number":1254,"context_line":"        # found before letting any work get done; so we should probably lower"},{"line_number":1255,"context_line":"        # that cache size on startup"},{"line_number":1256,"context_line":"        # stub_max_objects_to_cache \u003d 10"},{"line_number":1257,"context_line":"        stub_max_objects_to_cache \u003d None"},{"line_number":1258,"context_line":"        self._expirer_run_once_with_mocks("},{"line_number":1259,"context_line":"            now\u003dnow, stub_shuffle\u003dstub_shuffle,"}],"source_content_type":"text/x-python","patch_set":5,"id":"77dc916d_ce17cea5","line":1256,"in_reply_to":"5710c362_5d3440f6","updated":"2024-04-16 19:05:12.000000000","message":"Done","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"1f715c06fc1d3884f59c577f0bda895526ace43d","unresolved":true,"context_lines":[{"line_number":1253,"context_line":"        # round_robin_order will iter task_containers until *all* tasks are"},{"line_number":1254,"context_line":"        # found before letting any work get done; so we should probably lower"},{"line_number":1255,"context_line":"        # that cache size on startup"},{"line_number":1256,"context_line":"        # stub_max_objects_to_cache \u003d 10"},{"line_number":1257,"context_line":"        stub_max_objects_to_cache \u003d None"},{"line_number":1258,"context_line":"        self._expirer_run_once_with_mocks("},{"line_number":1259,"context_line":"            now\u003dnow, stub_shuffle\u003dstub_shuffle,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5710c362_5d3440f6","line":1256,"in_reply_to":"b42132e2_44935443","updated":"2024-04-05 18:36:19.000000000","message":"yes, in this change it was \"intentional\" (but obviously not entirely sufficient/helpful to subtley convey why this test takes forever).  The comment was ment to be a \"preview\" of the follow-on change:\n\nhttps://review.opendev.org/c/openstack/swift/+/914714/4/test/unit/obj/test_expirer.py#1256","commit_id":"7302e8e929942dfd61300ae3c0526771d0959936"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"5212cd3007098cf1c19fd356bbf4e447e5c05f78","unresolved":true,"context_lines":[{"line_number":1146,"context_line":"                account, i, next(obj_names))"},{"line_number":1147,"context_line":""},{"line_number":1148,"context_line":"        day_seconds \u003d 60 * 60 * 24"},{"line_number":1149,"context_line":"        self.conf[\u0027grace_period_%s\u0027 % accounts[0]] \u003d day_seconds * 10"},{"line_number":1150,"context_line":"        self._setup_fake_swift({"},{"line_number":1151,"context_line":"            \u0027.expiring_objects\u0027: {"},{"line_number":1152,"context_line":"                task_container: ["}],"source_content_type":"text/x-python","patch_set":6,"id":"abf3c726_ee20ccef","line":1149,"range":{"start_line":1149,"start_character":19,"end_line":1149,"end_character":32},"updated":"2024-04-09 16:47:14.000000000","message":"needs updating to delay_reaping to work after rebase on latest parent version","commit_id":"64aff91a2d9fd7e4ad399880557b41732133056d"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1146,"context_line":"                account, i, next(obj_names))"},{"line_number":1147,"context_line":""},{"line_number":1148,"context_line":"        day_seconds \u003d 60 * 60 * 24"},{"line_number":1149,"context_line":"        self.conf[\u0027grace_period_%s\u0027 % accounts[0]] \u003d day_seconds * 10"},{"line_number":1150,"context_line":"        self._setup_fake_swift({"},{"line_number":1151,"context_line":"            \u0027.expiring_objects\u0027: {"},{"line_number":1152,"context_line":"                task_container: ["}],"source_content_type":"text/x-python","patch_set":6,"id":"f0b6cd94_9a38442f","line":1149,"range":{"start_line":1149,"start_character":19,"end_line":1149,"end_character":32},"in_reply_to":"abf3c726_ee20ccef","updated":"2024-04-16 19:05:12.000000000","message":"Done","commit_id":"64aff91a2d9fd7e4ad399880557b41732133056d"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1105,"context_line":"        tasks for accounts[0] is in the queue."},{"line_number":1106,"context_line":""},{"line_number":1107,"context_line":"        :param now: the timestamp used consistently as now by the expirer"},{"line_number":1108,"context_line":"        :param num_days_past_due: number of task_container days \u003c\u003d now"},{"line_number":1109,"context_line":"        :param num_days_after_due: number of task_container days \u003e now"},{"line_number":1110,"context_line":"        :param containers_per_day: common.utils.get_expirer_container creates"},{"line_number":1111,"context_line":"                                   at most 100 containers per day by default"},{"line_number":1112,"context_line":"        :param num_tasks_per_container: the number of queue entries per"}],"source_content_type":"text/x-python","patch_set":7,"id":"78f2bf0a_f9941eb4","line":1109,"range":{"start_line":1108,"start_character":15,"end_line":1109,"end_character":34},"updated":"2024-04-09 18:28:58.000000000","message":"I found this naming confusing: to my mind \u0027past\u0027 and \u0027after\u0027 are the same. Could we use num_days_before_now and num_days_after_now?\n\nThe doc string does however give a precise definition of the args meaning 😊","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1105,"context_line":"        tasks for accounts[0] is in the queue."},{"line_number":1106,"context_line":""},{"line_number":1107,"context_line":"        :param now: the timestamp used consistently as now by the expirer"},{"line_number":1108,"context_line":"        :param num_days_past_due: number of task_container days \u003c\u003d now"},{"line_number":1109,"context_line":"        :param num_days_after_due: number of task_container days \u003e now"},{"line_number":1110,"context_line":"        :param containers_per_day: common.utils.get_expirer_container creates"},{"line_number":1111,"context_line":"                                   at most 100 containers per day by default"},{"line_number":1112,"context_line":"        :param num_tasks_per_container: the number of queue entries per"}],"source_content_type":"text/x-python","patch_set":7,"id":"8c630acf_c18e5ef2","line":1109,"range":{"start_line":1108,"start_character":15,"end_line":1109,"end_character":34},"in_reply_to":"78f2bf0a_f9941eb4","updated":"2024-04-16 19:05:12.000000000","message":"the new names you suggested were fine for me; suqashed.","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1123,"context_line":"            \u0027AUTH_a3\u0027,"},{"line_number":1124,"context_line":"        ] if accounts is None else accounts"},{"line_number":1125,"context_line":"        task_containers \u003d ["},{"line_number":1126,"context_line":"            str(int((now - (86400 * i)) - j))"},{"line_number":1127,"context_line":"            for i in range(num_days_past_due)"},{"line_number":1128,"context_line":"            for j in range(containers_per_day)"},{"line_number":1129,"context_line":"        ] + ["}],"source_content_type":"text/x-python","patch_set":7,"id":"617af6ca_b03c69c4","line":1126,"range":{"start_line":1126,"start_character":28,"end_line":1126,"end_character":33},"updated":"2024-04-09 18:28:58.000000000","message":"nit: day_seconds is defined below\n\nFWIW I\u0027m comfortable with 86400, it\u0027s become a well-known number for me, but  if day_seconds is a thing then it makes sense to use it throughout","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1123,"context_line":"            \u0027AUTH_a3\u0027,"},{"line_number":1124,"context_line":"        ] if accounts is None else accounts"},{"line_number":1125,"context_line":"        task_containers \u003d ["},{"line_number":1126,"context_line":"            str(int((now - (86400 * i)) - j))"},{"line_number":1127,"context_line":"            for i in range(num_days_past_due)"},{"line_number":1128,"context_line":"            for j in range(containers_per_day)"},{"line_number":1129,"context_line":"        ] + ["}],"source_content_type":"text/x-python","patch_set":7,"id":"2f9b09a8_2c9d4afd","line":1126,"range":{"start_line":1126,"start_character":28,"end_line":1126,"end_character":33},"in_reply_to":"617af6ca_b03c69c4","updated":"2024-04-16 19:05:12.000000000","message":"agreed; either is fine - but worth being consistent at least in this function.","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1138,"context_line":"        obj_names \u003d (\u0027obj%s\u0027 % i for i in itertools.count())"},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"        def task_name(task_container, i):"},{"line_number":1141,"context_line":"            account \u003d accounts[i % 3]"},{"line_number":1142,"context_line":"            # before *today* there\u0027s *only* grace account objects"},{"line_number":1143,"context_line":"            if Timestamp(task_container) \u003c now - containers_per_day:"},{"line_number":1144,"context_line":"                account \u003d accounts[0]"}],"source_content_type":"text/x-python","patch_set":7,"id":"2bdb162d_f17e249e","line":1141,"range":{"start_line":1141,"start_character":35,"end_line":1141,"end_character":36},"updated":"2024-04-09 18:28:58.000000000","message":"accounts[3] AUTH_a3 is not used?\n\nAlso, I\u0027m not sure why accounts would need to be passed in?","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":true,"context_lines":[{"line_number":1138,"context_line":"        obj_names \u003d (\u0027obj%s\u0027 % i for i in itertools.count())"},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"        def task_name(task_container, i):"},{"line_number":1141,"context_line":"            account \u003d accounts[i % 3]"},{"line_number":1142,"context_line":"            # before *today* there\u0027s *only* grace account objects"},{"line_number":1143,"context_line":"            if Timestamp(task_container) \u003c now - containers_per_day:"},{"line_number":1144,"context_line":"                account \u003d accounts[0]"}],"source_content_type":"text/x-python","patch_set":7,"id":"8984ffd9_7dfc8a5d","line":1141,"range":{"start_line":1141,"start_character":35,"end_line":1141,"end_character":36},"in_reply_to":"2bdb162d_f17e249e","updated":"2024-04-16 19:05:12.000000000","message":"\u003e accounts[3] AUTH_a3 is not used?\n\nyup, that\u0027s a bug - 3 here should be len(accounts)\n\n\u003e Also, I\u0027m not sure why accounts would need to be passed in?\n\nI understand this feedback as essentially: YAGNI - which is good advice whether you\u0027re creating a new helper in utils or new test infra.  I think I just have a cringe for test helpers that are SO opionated they require significant re-work to have ANY use out-side of the original context they were created in.  But the other extream is having 6 slightly different FakeX helpers - so I think I tend to be more tolerant of some technical investment in test-infra if it might encourage an existing test helper to be modified/extended to support a new use-case rather than a future test author concluding \"it\u0027s not worth it\" and rolling their own slightly different helper to \"setup a bunch of tasks\"","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0fdae2fdd0f32a0804a768616cb702d1db4b2d34","unresolved":false,"context_lines":[{"line_number":1138,"context_line":"        obj_names \u003d (\u0027obj%s\u0027 % i for i in itertools.count())"},{"line_number":1139,"context_line":""},{"line_number":1140,"context_line":"        def task_name(task_container, i):"},{"line_number":1141,"context_line":"            account \u003d accounts[i % 3]"},{"line_number":1142,"context_line":"            # before *today* there\u0027s *only* grace account objects"},{"line_number":1143,"context_line":"            if Timestamp(task_container) \u003c now - containers_per_day:"},{"line_number":1144,"context_line":"                account \u003d accounts[0]"}],"source_content_type":"text/x-python","patch_set":7,"id":"e289991d_5bd8b216","line":1141,"range":{"start_line":1141,"start_character":35,"end_line":1141,"end_character":36},"in_reply_to":"8984ffd9_7dfc8a5d","updated":"2024-04-18 16:03:11.000000000","message":"Done","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1168,"context_line":"            (num_days_past_due - 1) * containers_per_day"},{"line_number":1169,"context_line":"        ) + ("},{"line_number":1170,"context_line":"            # how many iters of useful work needed to bust the cache"},{"line_number":1171,"context_line":"            (max_objects_to_cache // non_grace_task_per_container) + 1"},{"line_number":1172,"context_line":"        )"},{"line_number":1173,"context_line":"        return (now,"},{"line_number":1174,"context_line":"                num_expected_iter_calls,"}],"source_content_type":"text/x-python","patch_set":7,"id":"0143c7f4_2d625df4","line":1171,"updated":"2024-04-09 18:28:58.000000000","message":"I managed to break things by increasing max_objects_to_cache - at some point (\u003e60) there\u0027s insufficient tasks in \"today\" to fill one object cache so this terms becomes capped at ``containers_per_day``","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1168,"context_line":"            (num_days_past_due - 1) * containers_per_day"},{"line_number":1169,"context_line":"        ) + ("},{"line_number":1170,"context_line":"            # how many iters of useful work needed to bust the cache"},{"line_number":1171,"context_line":"            (max_objects_to_cache // non_grace_task_per_container) + 1"},{"line_number":1172,"context_line":"        )"},{"line_number":1173,"context_line":"        return (now,"},{"line_number":1174,"context_line":"                num_expected_iter_calls,"}],"source_content_type":"text/x-python","patch_set":7,"id":"2df100dc_082ba737","line":1171,"in_reply_to":"0143c7f4_2d625df4","updated":"2024-04-16 19:05:12.000000000","message":"Acknowledged","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1194,"context_line":"            if stub_max_task_container_to_cache is not None"},{"line_number":1195,"context_line":"            else expirer.MAX_TASK_CONTAINER_TO_CACHE"},{"line_number":1196,"context_line":"        )"},{"line_number":1197,"context_line":"        # IME abuse of MagicMock\u0027s call tracking will pop OOM"},{"line_number":1198,"context_line":"        memory_efficient_noop \u003d lambda *args, **kwargs: None"},{"line_number":1199,"context_line":"        memory_efficient_time \u003d lambda: now"},{"line_number":1200,"context_line":"        with mock.patch(\u0027swift.obj.expirer.MAX_TASK_CONTAINER_TO_CACHE\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"bc1bcdeb_6fd06b3f","line":1197,"updated":"2024-04-09 18:28:58.000000000","message":"trying to understand this comment: is it that a simple mock.patch(\u0027eventlet.sleep\u0027) installs a MagicMock which tracks calls to sleep and therefore hits OOM? That may not be the case with the follow on that reduces the number of tasks?","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1194,"context_line":"            if stub_max_task_container_to_cache is not None"},{"line_number":1195,"context_line":"            else expirer.MAX_TASK_CONTAINER_TO_CACHE"},{"line_number":1196,"context_line":"        )"},{"line_number":1197,"context_line":"        # IME abuse of MagicMock\u0027s call tracking will pop OOM"},{"line_number":1198,"context_line":"        memory_efficient_noop \u003d lambda *args, **kwargs: None"},{"line_number":1199,"context_line":"        memory_efficient_time \u003d lambda: now"},{"line_number":1200,"context_line":"        with mock.patch(\u0027swift.obj.expirer.MAX_TASK_CONTAINER_TO_CACHE\u0027,"}],"source_content_type":"text/x-python","patch_set":7,"id":"e37aa9af_30f84699","line":1197,"in_reply_to":"bc1bcdeb_6fd06b3f","updated":"2024-04-16 19:05:12.000000000","message":"you\u0027ve got it - with less tasks the OOM doesn\u0027t happen.","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"f2e6ab80a87032939d25b4035ace1370c331807e","unresolved":true,"context_lines":[{"line_number":1283,"context_line":"            self.expirer.run_once()"},{"line_number":1284,"context_line":""},{"line_number":1285,"context_line":"        # old mock doesn\u0027t suport c.args the same way as new mock"},{"line_number":1286,"context_line":"        expected \u003d {c[0] for c in mock_method.call_args_list}"},{"line_number":1287,"context_line":"        # iter_objects is called only for past_time, not future_time"},{"line_number":1288,"context_line":"        self.assertEqual(expected, {"},{"line_number":1289,"context_line":"            (\u0027.expiring_objects\u0027, self.empty_time),"}],"source_content_type":"text/x-python","patch_set":7,"id":"7bcd4a0d_acd45337","line":1286,"updated":"2024-04-09 18:28:58.000000000","message":"these are the actual calls rather then the expected","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"6bcd132ebdfbb2b25da9c40fbeb4fdd8ed812bad","unresolved":false,"context_lines":[{"line_number":1283,"context_line":"            self.expirer.run_once()"},{"line_number":1284,"context_line":""},{"line_number":1285,"context_line":"        # old mock doesn\u0027t suport c.args the same way as new mock"},{"line_number":1286,"context_line":"        expected \u003d {c[0] for c in mock_method.call_args_list}"},{"line_number":1287,"context_line":"        # iter_objects is called only for past_time, not future_time"},{"line_number":1288,"context_line":"        self.assertEqual(expected, {"},{"line_number":1289,"context_line":"            (\u0027.expiring_objects\u0027, self.empty_time),"}],"source_content_type":"text/x-python","patch_set":7,"id":"13182465_84c87d9e","line":1286,"in_reply_to":"7bcd4a0d_acd45337","updated":"2024-04-16 19:05:12.000000000","message":"indeed!  🙏","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3857a860766e667ee312fa7cce0a3113c4c7ade7","unresolved":true,"context_lines":[{"line_number":1378,"context_line":"                               fail_delete_container), \\"},{"line_number":1379,"context_line":"                mock.patch.object(self.expirer, \u0027delete_actual_object\u0027,"},{"line_number":1380,"context_line":"                                  fail_delete_actual_object), \\"},{"line_number":1381,"context_line":"                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None), \\"},{"line_number":1382,"context_line":"                mock.patch.object(self.expirer, \u0027pop_queue\u0027) as mock_pop:"},{"line_number":1383,"context_line":"            self.expirer.run_once()"},{"line_number":1384,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"98bca90e_debc07d9","line":1381,"updated":"2024-04-19 14:24:13.000000000","message":"in retrospect it seems that *none* of these mock shuffles were needed as long as the task size was less than the round robin cache size - it really WASN\u0027T the randomized task container iteration that makes them unstable - it\u0027s was the removal of the stable ordering of round-robin and the fact that all tasks created in setup have a different target containers.","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[{"line_number":1378,"context_line":"                               fail_delete_container), \\"},{"line_number":1379,"context_line":"                mock.patch.object(self.expirer, \u0027delete_actual_object\u0027,"},{"line_number":1380,"context_line":"                                  fail_delete_actual_object), \\"},{"line_number":1381,"context_line":"                mock.patch(\u0027swift.obj.expirer.shuffle\u0027, lambda a: None), \\"},{"line_number":1382,"context_line":"                mock.patch.object(self.expirer, \u0027pop_queue\u0027) as mock_pop:"},{"line_number":1383,"context_line":"            self.expirer.run_once()"},{"line_number":1384,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"265a0c51_e41e8972","line":1381,"in_reply_to":"98bca90e_debc07d9","updated":"2024-06-04 18:16:27.000000000","message":"Acknowledged","commit_id":"4107f58699688085fc521b06a476969d834e3d0b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d5fa76291e6279689212adc16ee7ff04bd0d3275","unresolved":true,"context_lines":[{"line_number":1302,"context_line":"        now \u003d time()"},{"line_number":1303,"context_line":"        num_days_before_now \u003d 7"},{"line_number":1304,"context_line":"        num_days_after_now \u003d 3"},{"line_number":1305,"context_line":"        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027true\u0027"},{"line_number":1306,"context_line":"        self.conf[\u0027round_robin_task_cache_size\u0027] \u003d 200"},{"line_number":1307,"context_line":"        containers_per_day \u003d 10"},{"line_number":1308,"context_line":"        num_tasks_per_container \u003d 100"}],"source_content_type":"text/x-python","patch_set":9,"id":"d8274197_30bba8ed","line":1305,"updated":"2024-04-17 15:15:16.000000000","message":"it might be weird to have this helper take a stub_shuffle option that potentially has no effect if the caller didn\u0027t configure this option?","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[{"line_number":1302,"context_line":"        now \u003d time()"},{"line_number":1303,"context_line":"        num_days_before_now \u003d 7"},{"line_number":1304,"context_line":"        num_days_after_now \u003d 3"},{"line_number":1305,"context_line":"        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027true\u0027"},{"line_number":1306,"context_line":"        self.conf[\u0027round_robin_task_cache_size\u0027] \u003d 200"},{"line_number":1307,"context_line":"        containers_per_day \u003d 10"},{"line_number":1308,"context_line":"        num_tasks_per_container \u003d 100"}],"source_content_type":"text/x-python","patch_set":9,"id":"0347f04c_1caaf729","line":1305,"in_reply_to":"6fc2fc62_a8a08033","updated":"2024-06-04 18:16:27.000000000","message":"Done","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0fdae2fdd0f32a0804a768616cb702d1db4b2d34","unresolved":true,"context_lines":[{"line_number":1302,"context_line":"        now \u003d time()"},{"line_number":1303,"context_line":"        num_days_before_now \u003d 7"},{"line_number":1304,"context_line":"        num_days_after_now \u003d 3"},{"line_number":1305,"context_line":"        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027true\u0027"},{"line_number":1306,"context_line":"        self.conf[\u0027round_robin_task_cache_size\u0027] \u003d 200"},{"line_number":1307,"context_line":"        containers_per_day \u003d 10"},{"line_number":1308,"context_line":"        num_tasks_per_container \u003d 100"}],"source_content_type":"text/x-python","patch_set":9,"id":"6fc2fc62_a8a08033","line":1305,"in_reply_to":"d8274197_30bba8ed","updated":"2024-04-18 16:03:11.000000000","message":"I think if this method always enables randomized_task_queue_iteration when the caller provides a stub_shuffle then it might be reasonable for a caller to get default behavior by passing stub_shuffle\u003dNone ... maybe?\n\nWhat I\u0027d like to avoid is a test helper interface that has an obviously wrong way to call it like:\n\n    _do_test(config_randomized\u003dFalse, shuffle\u003dmy_mock)\n    \nof\n\n    # self.conf[\u0027randomized\u0027] \u003d False\n    _do_test(shuffle\u003dmy_mock)\n    \n... where my_mock has no \"effect\" because randomization it\u0027s disabled\n\nUnless *maybe* if this helper wanted to enable a caller that was trying to sanity check the config behavior and wanted to `assertFalse(my_mock.called)` - except that doesn\u0027t even work because of a weird side-effect of how the disabled behavior is implemented with a call to shuffle on the empty list at the end of the loop...\n\nI think this helper is *best* understood as a test for the new \"randomized_task_container_iteration \u003d true\" behavior and it\u0027s really a bit of a strech to repurpose it for assertions on the \"randomized_task_container_iteration \u003d false\" behavior; since the default behavior has no randomization it just seems like there\u0027d be easier ways to make statements about the expected behavior.  And also \u0027assert worst case\" isn\u0027t even the particular behavior we\u0027d be trying to maintain with the default of \"false\"... the desirable behavior of the default would be more like \"deletes oldest from backlog ASAP\" - which would kind of be a different task queue or at least different assertions; we could maybe write that test *too* - if it\u0027s needed?\n\nidk, i\u0027m sure there\u0027s many ways to do write it...","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2669fa59477774972450d6ec4a73ac4df735f879","unresolved":true,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"2f28cee5_b86b42ec","line":1358,"updated":"2024-04-17 14:25:47.000000000","message":"this is now the default conf so we shouldn\u0027t need need to mock\n\n```\ndiff --git a/test/unit/obj/test_expirer.py b/test/unit/obj/test_expirer.py\nindex f92aadb2e..a424ef729 100644\n--- a/test/unit/obj/test_expirer.py\n+++ b/test/unit/obj/test_expirer.py\n@@ -1302,7 +1302,6 @@ class TestObjectExpirer(TestCase):\n         now \u003d time()\n         num_days_before_now \u003d 7\n         num_days_after_now \u003d 3\n-        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027true\u0027\n         self.conf[\u0027round_robin_task_cache_size\u0027] \u003d 200\n         containers_per_day \u003d 10\n         num_tasks_per_container \u003d 100\n@@ -1355,14 +1354,16 @@ class TestObjectExpirer(TestCase):\n\n     def test_in_order_task_container_iteration_delays_deletes(self):\n         # \"in order\" is the least optimized \"randomization\"\n-        stub_shuffle \u003d lambda x: None\n+        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027false\u0027\n         best_case, observed, worst_case \u003d \\\n-            self._do_test_task_queue_iteration_order(stub_shuffle)\n+            self._do_test_task_queue_iteration_order(None)\n         print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))\n         self.assertEqual(observed, worst_case)\n\n     def test_reversed_task_container_iteration_hastens_deletes(self):\n         # \"shuffle newer to front\" is the most optimized \"randomization\"\n+        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027true\u0027\n+\n         def stub_shuffle(a):\n             a.reverse()\n         best_case, observed, worst_case \u003d \\\n@@ -1372,6 +1373,7 @@ class TestObjectExpirer(TestCase):\n\n     def test_randomized_task_container_iteration_hastens_deletes(self):\n         # randomized sits in the middle\n+        self.conf[\u0027randomized_task_queue_iteration\u0027] \u003d \u0027true\u0027\n         best_case, observed, worst_case \u003d \\\n             self._do_test_task_queue_iteration_order(None)\n         print(\u0027randomized: %s \u003c\u003d %s \u003c %s\u0027 % (best_case, observed, worst_case))\n```","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"a832c1607e8be8cb976d3d0b3601062a2f136bdc","unresolved":true,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"8d365024_cb0bdb36","line":1358,"in_reply_to":"162be038_e48d2745","updated":"2024-04-19 12:45:50.000000000","message":"\"obtuse\n: lacking sharpness or quickness of sensibility or intellect : insensitive, stupid. He is too obtuse to take a hint. b. : difficult to comprehend : not clear or precise in thought or expression.\"\n\nHard not to take that as an insult. I\u0027ll keep my comments to the code.\n\nWith this diff on patchset 10 all tests in test_expirer.py passed:\n\n```\ndiff --git a/swift/obj/expirer.py b/swift/obj/expirer.py\nindex 6c1f8d65f..bb595c3f8 100644\n--- a/swift/obj/expirer.py\n+++ b/swift/obj/expirer.py\n@@ -304,10 +304,6 @@ class ObjectExpirer(Daemon):\n             timestamp \u003d self.delete_at_time_of_task_container(task_container)\n             if timestamp \u003e Timestamp.now():\n                 break\n-            if not self.randomized_task_container_iteration:\n-                yield task_account, task_container\n-                # we never append to container_list\n-                continue\n             container_list.append(task_container)\n             if len(container_list) \u003e MAX_TASK_CONTAINER_TO_CACHE:\n                 shuffle(container_list)\n```","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0fdae2fdd0f32a0804a768616cb702d1db4b2d34","unresolved":true,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"162be038_e48d2745","line":1358,"in_reply_to":"1fd9ed7b_cf62dae7","updated":"2024-04-18 16:03:11.000000000","message":"\u003e I don\u0027t see a test that asserts that running with the default option gets you worst case time-to-first-delete.\n\nDon\u0027t be obtuse, I ment there were existing tests that execute on the default non-randomized configuration with in order behavior and assert on the order of listings on that stable order - not specifically that they asserted the start-up delay was equivilent to degenerate in-order shuffle.\n\nDespite the configurability of the randomization behavior, I feel a \"test_in_order_task_container_iteration_delays_deletes\" that shows a \"shuffle\" of \"in-order\" is the MOST effective way to contrast the \"opposite\" of \"test_reversed_task_container_iteration_hastens_deletes\" which demonstrates a \"shuffle\" of \"reverse\".\n\nHowever you\u0027ve convinced me it could ALSO be valuable to have a test that demonstrates the equivilence of \"in-order shuffle\" and \"randomized_task_container_iteration turned off\" - I\u0027ll add that.","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d5fa76291e6279689212adc16ee7ff04bd0d3275","unresolved":true,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"fbccf233_04f034c4","line":1358,"in_reply_to":"2f28cee5_b86b42ec","updated":"2024-04-17 15:15:16.000000000","message":"heh, what happened to keeping the test\u0027s setup consistent!?\n\nI think \"you turned on random but your shuffle got unlucky\" is kind of a different test than \"you didn\u0027t configure randomized_task_iteration\" (which is tested else where)\n\nI think this code could change like the diff you posted and still also be correct - but are you recommending that would be BETTER or just an alternative?","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"260d9b22_0838089c","line":1358,"in_reply_to":"50d7e682_a519f173","updated":"2024-06-04 18:16:27.000000000","message":"Done","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3857a860766e667ee312fa7cce0a3113c4c7ade7","unresolved":true,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"50d7e682_a519f173","line":1358,"in_reply_to":"8d365024_cb0bdb36","updated":"2024-04-19 14:24:13.000000000","message":"\u003e Hard not to take that as an insult\n\nmy sincere apologies for giving insult.  FWIW in my vernacular there\u0027s a connotation of intention when \"being obtuse\", like \"playing dumb\".  I think I was getting weary of the long cycle time for the many iterations we\u0027ve had on this test.  I honestly can see now I SHOULD have found a more generous way to interpret you saying you \"don\u0027t SEE a test\" which I\u0027d just claimed was \"tested else where\" than to imagine you were \"playing dumb\".  I think I\u0027d had been carrying around a misunderstanding that at least 3 or 4 of the existing tests had assertions that were dependent on the order of task_container iteration and it just wasn\u0027t so.  Regardless, I certainly should take make care with my words.  I\u0027m sorry.\n\n\u003e With this diff on patchset 10 all tests in test_expirer.py passed:\n\nthat you, that was enlightening!\n\nIn pathset 13 multiple tests break with that diff\n\n```\nFAILED swift/test/unit/obj/test_expirer.py::TestObjectExpirer::test_container_timestamp_break - AssertionError: [call[21 chars] \u00271713535840\u0027),\nFAILED swift/test/unit/obj/test_expirer.py::TestObjectExpirer::test_default_task_container_iteration_delays_deletes - AssertionError: 42 !\u003d 64\n```","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"900188ecf05682c07cdf88ea68b77c13943632a7","unresolved":true,"context_lines":[{"line_number":1355,"context_line":""},{"line_number":1356,"context_line":"    def test_in_order_task_container_iteration_delays_deletes(self):"},{"line_number":1357,"context_line":"        # \"in order\" is the least optimized \"randomization\""},{"line_number":1358,"context_line":"        stub_shuffle \u003d lambda x: None"},{"line_number":1359,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1360,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1361,"context_line":"        print(\u0027in order: %s \u003d\u003d %s\u0027 % (observed, worst_case))"}],"source_content_type":"text/x-python","patch_set":9,"id":"1fd9ed7b_cf62dae7","line":1358,"in_reply_to":"fbccf233_04f034c4","updated":"2024-04-18 08:37:13.000000000","message":"\u003e heh, what happened to keeping the test\u0027s setup consistent!?\n\nI don\u0027t see the inconsistency in the test other than varying the conf option, which is the lever that ops have.\n\n\u003e I think \"you turned on random but your shuffle got unlucky\" is kind of a different test than \"you didn\u0027t configure randomized_task_iteration\" (which is tested else where)\n\nI don\u0027t see a test that asserts that running with the default option gets you worst case time-to-first-delete.\n\nI\u0027m suggesting that this could be that test.\n\nI think a comment to the effect that \"even with random turned on you may get unlucky and still get worst case which is equivalent to random off\" would be sufficient and great. In fact, maybe that kind of comment should be in the config doc.\n\nIMHO that would be better, but I\u0027m keen to understand why you see it differently.","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"900188ecf05682c07cdf88ea68b77c13943632a7","unresolved":true,"context_lines":[{"line_number":1376,"context_line":"            self._do_test_task_queue_iteration_order(None)"},{"line_number":1377,"context_line":"        print(\u0027randomized: %s \u003c\u003d %s \u003c %s\u0027 % (best_case, observed, worst_case))"},{"line_number":1378,"context_line":"        self.assertGreaterEqual(observed, best_case)"},{"line_number":1379,"context_line":"        self.assertLess(observed, worst_case)"},{"line_number":1380,"context_line":""},{"line_number":1381,"context_line":"    def test_run_once_unicode_problem(self):"},{"line_number":1382,"context_line":"        requests \u003d []"}],"source_content_type":"text/x-python","patch_set":9,"id":"8e33b9be_84df6bec","line":1379,"updated":"2024-04-18 08:37:13.000000000","message":"This has to be assertLessEqual to be consistent with the observation that \"you may get unlucky\". Which sucks because that makes the assertions indistinct w.r.t the other two test cases. We could set the random seed to force a predictable outcome.","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[{"line_number":1376,"context_line":"            self._do_test_task_queue_iteration_order(None)"},{"line_number":1377,"context_line":"        print(\u0027randomized: %s \u003c\u003d %s \u003c %s\u0027 % (best_case, observed, worst_case))"},{"line_number":1378,"context_line":"        self.assertGreaterEqual(observed, best_case)"},{"line_number":1379,"context_line":"        self.assertLess(observed, worst_case)"},{"line_number":1380,"context_line":""},{"line_number":1381,"context_line":"    def test_run_once_unicode_problem(self):"},{"line_number":1382,"context_line":"        requests \u003d []"}],"source_content_type":"text/x-python","patch_set":9,"id":"442493f3_6c1c606d","line":1379,"in_reply_to":"7811f33b_4be45b81","updated":"2024-06-04 18:16:27.000000000","message":"Acknowledged","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"0fdae2fdd0f32a0804a768616cb702d1db4b2d34","unresolved":true,"context_lines":[{"line_number":1376,"context_line":"            self._do_test_task_queue_iteration_order(None)"},{"line_number":1377,"context_line":"        print(\u0027randomized: %s \u003c\u003d %s \u003c %s\u0027 % (best_case, observed, worst_case))"},{"line_number":1378,"context_line":"        self.assertGreaterEqual(observed, best_case)"},{"line_number":1379,"context_line":"        self.assertLess(observed, worst_case)"},{"line_number":1380,"context_line":""},{"line_number":1381,"context_line":"    def test_run_once_unicode_problem(self):"},{"line_number":1382,"context_line":"        requests \u003d []"}],"source_content_type":"text/x-python","patch_set":9,"id":"7811f33b_4be45b81","line":1379,"in_reply_to":"8e33b9be_84df6bec","updated":"2024-04-18 16:03:11.000000000","message":"so we end up with 70 task_containers to scan (num_days_before_now * containers_per_day), and with tasks_per_container \u003d\u003d 100 and 3/7 tasks under grace we get about 57 actionable tasks per container, so @ 200 task_cache_size we need 4 of the 10 \"today\" containers to show up before we issue the first delete.\n\nSo that\u0027s why 4 is best case and 64 is the worst case, 60 stale containers + 4 from today.\n\nNow, there\u0027s about a 50% chance the first 4 containers are from the last 10:\n\n    (10/70) + (9/69) + (8/68) + (7/67) ~\u003d .5\n\n^ so that\u0027s why GreaterEqual makes sense, it\u0027s quite likely to hit best case.\n\nOTOH, there\u0027s ~1.2e100 permutations of the task containers (70!), and also about ~1.2e100 permutations where at least 4 of the last 10 show up in the first 60 (because there\u0027s only 210 ways so choose the wrong 6 from 10) - i.e. assuming the pseudo random algorithm is reasonable if we ran this continuously every second for 1000x the life of the universe we might expect it to hit a worst case; but we\u0027re talking about a \"one in a googol\" kind of odds.\n\n^ so that\u0027s why Less makes sense, it\u0027s not particularlly likely except in the contrived/degenerate case.","commit_id":"baaf3bd30e487d15ab6936d20e13fa6e60f10e95"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3857a860766e667ee312fa7cce0a3113c4c7ade7","unresolved":true,"context_lines":[{"line_number":1414,"context_line":"        self.assertEqual(captured, {"},{"line_number":1415,"context_line":"            (\u0027.expiring_objects\u0027, self.empty_time),"},{"line_number":1416,"context_line":"            (\u0027.expiring_objects\u0027, self.past_time),"},{"line_number":1417,"context_line":"            (\u0027.expiring_objects\u0027, self.just_past_time)})"},{"line_number":1418,"context_line":""},{"line_number":1419,"context_line":"    def test_object_timestamp_break(self):"},{"line_number":1420,"context_line":"        with mock.patch.object(self.expirer, \u0027delete_actual_object\u0027) \\"}],"source_content_type":"text/x-python","patch_set":12,"id":"03acefb0_2e705cc2","line":1417,"updated":"2024-04-19 14:24:13.000000000","message":"this should have been reverted when I pulled the mock shuffles; since randomization is no longer configured in this test the order is once again stable.","commit_id":"5652a06b7b7b04579c7c4c9a998d341c7dfc7c8c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"492566ebcd745c47460f37a3455bb8437d602d9b","unresolved":false,"context_lines":[{"line_number":1414,"context_line":"        self.assertEqual(captured, {"},{"line_number":1415,"context_line":"            (\u0027.expiring_objects\u0027, self.empty_time),"},{"line_number":1416,"context_line":"            (\u0027.expiring_objects\u0027, self.past_time),"},{"line_number":1417,"context_line":"            (\u0027.expiring_objects\u0027, self.just_past_time)})"},{"line_number":1418,"context_line":""},{"line_number":1419,"context_line":"    def test_object_timestamp_break(self):"},{"line_number":1420,"context_line":"        with mock.patch.object(self.expirer, \u0027delete_actual_object\u0027) \\"}],"source_content_type":"text/x-python","patch_set":12,"id":"fec1dce0_a8e63a4b","line":1417,"in_reply_to":"03acefb0_2e705cc2","updated":"2024-06-04 18:16:27.000000000","message":"Done","commit_id":"5652a06b7b7b04579c7c4c9a998d341c7dfc7c8c"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"18c32a6a6e97877622d0c26f9b334fb65e40e726","unresolved":false,"context_lines":[{"line_number":291,"context_line":"    def _setup_fake_swift(self, aco_dict):"},{"line_number":292,"context_line":"        self.fake_swift \u003d FakeInternalClient(aco_dict)"},{"line_number":293,"context_line":"        self.expirer \u003d expirer.ObjectExpirer(self.conf, logger\u003dself.logger,"},{"line_number":294,"context_line":"                                             swift\u003dself.fake_swift)"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"    def make_fake_ic(self, app):"},{"line_number":297,"context_line":"        app._pipeline_final_app \u003d mock.MagicMock()"}],"source_content_type":"text/x-python","patch_set":22,"id":"f9bc1eee_365fc3f1","line":294,"updated":"2024-06-14 01:27:10.000000000","message":"this was super useful - it go pulled into an earlier patch","commit_id":"193cffa6863064d8452351e314cbee69bdb9cd3f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"18c32a6a6e97877622d0c26f9b334fb65e40e726","unresolved":false,"context_lines":[{"line_number":1710,"context_line":"        best_case, observed, worst_case \u003d \\"},{"line_number":1711,"context_line":"            self._do_test_task_queue_iteration_order(stub_shuffle)"},{"line_number":1712,"context_line":"        print(\u0027randomized: %s \u003c\u003d %s \u003c %s\u0027 % (best_case, observed, worst_case))"},{"line_number":1713,"context_line":"        self.assertGreaterEqual(observed, best_case)"},{"line_number":1714,"context_line":"        self.assertLess(observed, worst_case)"},{"line_number":1715,"context_line":""},{"line_number":1716,"context_line":"    def test_run_once_unicode_problem(self):"}],"source_content_type":"text/x-python","patch_set":22,"id":"0f40bcb9_fb523bd0","line":1713,"updated":"2024-06-14 01:27:10.000000000","message":"huh, observed:\n\n    AssertionError: 3 not greater than or equal to 4\n\nbut then couldn\u0027t reproduce in 100 runs - maybe it was just an inconsistent shared filesystem sort of thing?","commit_id":"193cffa6863064d8452351e314cbee69bdb9cd3f"}]}