)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3e399ca52bbe4d6fa7044fc845533f15ffbfa2f1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"21520b60_d01e2be1","updated":"2023-07-05 23:32:22.000000000","message":"this seems reasonable and under-tested\n\nI found an existing test that\u0027s relevant to this condition:\n\n\tpytest swift/test/unit/common/middleware/test_object_versioning.py::ObjectVersioningTestContainerOperations::test_list_versions_404_versions_container -vsx\n\n\nbut when I tried to assert we get a HEAD instead of a GET it still showed the HEAD?  Maybe test test has memcache or get_container_info faked up?\n\n\tdiff --git a/test/unit/common/middleware/test_object_versioning.py b/test/unit/common/middleware/test_object_versioning.py\n\tindex 691d93fbc..d9e16396f 100644\n\t--- a/test/unit/common/middleware/test_object_versioning.py\n\t+++ b/test/unit/common/middleware/test_object_versioning.py\n\t@@ -2998,6 +2998,10 @@ class ObjectVersioningTestContainerOperations(ObjectVersioningBaseTestCase):\n\t\t     \u0027is_latest\u0027: True,\n\t\t }]\n\t\t self.assertEqual(expected, json.loads(body))\n\t+        self.assertEqual(self.app.calls, [\n\t+            (\u0027GET\u0027, \u0027/v1/a/c?format\u003djson\u0027),\n\t+            (\u0027GET\u0027, \u0027/v1/a/\\x00versions\\x00c?\u0027),\n\t+        ])\n\t \n\t     def test_bytes_count(self):\n\t\t self.app.register(","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ad82d37106dfff5c29f8bafc7a96ede372c1f9f9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"846ba6fd_e6155186","updated":"2023-07-06 18:44:26.000000000","message":"I\u0027m still positive on this change and a little perplexed by the testing infra.  Probably deserves another deeper look when I get a chance.","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"11942a393115be8a88b6d4d8bc4845c8b9deb4a6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"b75ac4bd_f44fc74a","updated":"2023-07-26 01:59:36.000000000","message":"Yup, nice improvement on dealing with expensive 404s and love how it can skip even the extra HEAD if it happens to be in cache, even just the info cache.","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"6ffdfdc74a1f05a513d413fab2cfd21c3716c08d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"c4bba78c_78dbeb78","updated":"2023-07-06 22:42:27.000000000","message":"recheck\n\nTests all passed, coverage was installed -- IDK why the coverage report wouldn\u0027t have been generated 😕","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"}],"swift/common/middleware/versioned_writes/object_versioning.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3e399ca52bbe4d6fa7044fc845533f15ffbfa2f1","unresolved":true,"context_lines":[{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        # NB: no end_marker support (yet)"},{"line_number":1168,"context_line":"        if get_container_info(versions_req.environ, self.app,"},{"line_number":1169,"context_line":"                              swift_source\u003d\u0027OV\u0027)[\u0027status\u0027] \u003d\u003d 404:"},{"line_number":1170,"context_line":"            # we don\u0027t usually like to LBYL like this, but 404s tend to be"},{"line_number":1171,"context_line":"            # expensive (since we check all primaries and a bunch of handoffs)"},{"line_number":1172,"context_line":"            # and we expect this to be a reasonably common way to listing"}],"source_content_type":"text/x-python","patch_set":1,"id":"18aca6a8_97026c1e","line":1169,"updated":"2023-07-05 23:32:22.000000000","message":"oic, and versions_req will have the \\x00versions container, so we don\u0027t really care/expect there to be any useful metadata in a cache hit or success response.\n\nIf the extra HEAD is a hit (cheap anyway) then we do the GET (also cheap).\n\nIf the HEAD is expensive (404/handoffs) then we cache that and skip the versions requests for awhile.","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7551543b41f9f2e6d3ce373fc0a5ec88a7002fca","unresolved":true,"context_lines":[{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        # NB: no end_marker support (yet)"},{"line_number":1168,"context_line":"        if get_container_info(versions_req.environ, self.app,"},{"line_number":1169,"context_line":"                              swift_source\u003d\u0027OV\u0027)[\u0027status\u0027] \u003d\u003d 404:"},{"line_number":1170,"context_line":"            # we don\u0027t usually like to LBYL like this, but 404s tend to be"},{"line_number":1171,"context_line":"            # expensive (since we check all primaries and a bunch of handoffs)"},{"line_number":1172,"context_line":"            # and we expect this to be a reasonably common way to listing"}],"source_content_type":"text/x-python","patch_set":1,"id":"db02b091_7045996e","line":1169,"in_reply_to":"18aca6a8_97026c1e","updated":"2023-07-06 19:30:40.000000000","message":"IDK that I\u0027d call the GET \"cheap,\" even if it\u0027s a hit -- could be a sharded container, for example. But yeah, having a HEAD that goes out to handoffs then gets cached seems **way** better than a GET that can\u0027t be cached.","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"24090df3df066ceb062a5f780d4445dd4ba3c774","unresolved":true,"context_lines":[{"line_number":1166,"context_line":""},{"line_number":1167,"context_line":"        # NB: no end_marker support (yet)"},{"line_number":1168,"context_line":"        if get_container_info(versions_req.environ, self.app,"},{"line_number":1169,"context_line":"                              swift_source\u003d\u0027OV\u0027)[\u0027status\u0027] \u003d\u003d 404:"},{"line_number":1170,"context_line":"            # we don\u0027t usually like to LBYL like this, but 404s tend to be"},{"line_number":1171,"context_line":"            # expensive (since we check all primaries and a bunch of handoffs)"},{"line_number":1172,"context_line":"            # and we expect this to be a reasonably common way to listing"}],"source_content_type":"text/x-python","patch_set":1,"id":"8966adbb_def228f4","line":1169,"in_reply_to":"db02b091_7045996e","updated":"2023-07-07 23:03:08.000000000","message":"404 container info in memcache expires 6 seconds by default, this approach will work for busy containers. I feel we can also remove below cache_time adjustment in set_info_cache()?\n    if resp.status_int in (HTTP_NOT_FOUND, HTTP_GONE):\n        cache_time *\u003d 0.1","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3e399ca52bbe4d6fa7044fc845533f15ffbfa2f1","unresolved":true,"context_lines":[{"line_number":1177,"context_line":"            versions_req.params \u003d {"},{"line_number":1178,"context_line":"                k: params.get(k, \u0027\u0027) for k in ("},{"line_number":1179,"context_line":"                    \u0027prefix\u0027, \u0027marker\u0027, \u0027limit\u0027, \u0027delimiter\u0027, \u0027reverse\u0027)}"},{"line_number":1180,"context_line":"            versions_resp \u003d versions_req.get_response(self.app)"},{"line_number":1181,"context_line":""},{"line_number":1182,"context_line":"        if versions_resp is None \\"},{"line_number":1183,"context_line":"                or versions_resp.status_int \u003d\u003d HTTP_NOT_FOUND:"}],"source_content_type":"text/x-python","patch_set":1,"id":"5c295eb8_37673261","line":1180,"updated":"2023-07-05 23:32:22.000000000","message":"so *this* is the extra request we think we can skip - if we have a 404 cache entry","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3e399ca52bbe4d6fa7044fc845533f15ffbfa2f1","unresolved":true,"context_lines":[{"line_number":1278,"context_line":"                self.update_content_length(len(body))"},{"line_number":1279,"context_line":"                app_resp \u003d [body]"},{"line_number":1280,"context_line":"        else:"},{"line_number":1281,"context_line":"            return versions_resp(versions_req.environ, start_response)"},{"line_number":1282,"context_line":""},{"line_number":1283,"context_line":"        start_response(self._response_status,"},{"line_number":1284,"context_line":"                       self._response_headers,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5b3f7eca_e7dabaad","line":1281,"updated":"2023-07-05 23:32:22.000000000","message":"I don\u0027t think we can get to the elif/else branch if versions_req \u003d None","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7551543b41f9f2e6d3ce373fc0a5ec88a7002fca","unresolved":true,"context_lines":[{"line_number":1278,"context_line":"                self.update_content_length(len(body))"},{"line_number":1279,"context_line":"                app_resp \u003d [body]"},{"line_number":1280,"context_line":"        else:"},{"line_number":1281,"context_line":"            return versions_resp(versions_req.environ, start_response)"},{"line_number":1282,"context_line":""},{"line_number":1283,"context_line":"        start_response(self._response_status,"},{"line_number":1284,"context_line":"                       self._response_headers,"}],"source_content_type":"text/x-python","patch_set":1,"id":"4337cca1_4ae2c4b8","line":1281,"in_reply_to":"5b3f7eca_e7dabaad","updated":"2023-07-06 19:30:40.000000000","message":"Correct -- maybe I should reword as something like\n```\nif versions_resp and not is_success(versions_resp.status_int):\n    return versions_resp(...)\n\nif not versions_resp or versions_resp.status_int \u003d\u003d 404:\n    ...\nelse:\n    ...\n```\n? \\*shrug\\*","commit_id":"8e5137d06c766ecab5ac0aa41347bb6a2ce2680b"}],"test/unit/common/middleware/test_object_versioning.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ad82d37106dfff5c29f8bafc7a96ede372c1f9f9","unresolved":true,"context_lines":[{"line_number":105,"context_line":"        self.cache_version_never_on \u003d FakeMemcache()"},{"line_number":106,"context_line":"        self.cache_version_never_on.set(get_cache_key(\u0027a\u0027), {\u0027status\u0027: 200})"},{"line_number":107,"context_line":"        self.cache_version_never_on.set(get_cache_key(\u0027a\u0027, \u0027c\u0027),"},{"line_number":108,"context_line":"                                        {\u0027status\u0027: 200})"},{"line_number":109,"context_line":"        self.expected_unread_requests \u003d {}"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def tearDown(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"4c59555b_e8c43192","line":108,"updated":"2023-07-06 18:44:26.000000000","message":"nit: ok, so there\u0027s an existing pattern of sharing the cache stub setup we want between tests - and there\u0027s a handful of different initial cache states prepared for the test to choose from.  This was a big wtf for me, thanks for figuring it out.","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7551543b41f9f2e6d3ce373fc0a5ec88a7002fca","unresolved":true,"context_lines":[{"line_number":105,"context_line":"        self.cache_version_never_on \u003d FakeMemcache()"},{"line_number":106,"context_line":"        self.cache_version_never_on.set(get_cache_key(\u0027a\u0027), {\u0027status\u0027: 200})"},{"line_number":107,"context_line":"        self.cache_version_never_on.set(get_cache_key(\u0027a\u0027, \u0027c\u0027),"},{"line_number":108,"context_line":"                                        {\u0027status\u0027: 200})"},{"line_number":109,"context_line":"        self.expected_unread_requests \u003d {}"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def tearDown(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"d84f16bf_2eada462","line":108,"in_reply_to":"4c59555b_e8c43192","updated":"2023-07-06 19:30:40.000000000","message":"Yeah, IDK that I entirely like doing things the way I did, but w/e","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ad82d37106dfff5c29f8bafc7a96ede372c1f9f9","unresolved":true,"context_lines":[{"line_number":3027,"context_line":"        req \u003d Request.blank("},{"line_number":3028,"context_line":"            \u0027/v1/a/c?versions\u0027,"},{"line_number":3029,"context_line":"            environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027GET\u0027,"},{"line_number":3030,"context_line":"                     \u0027swift.cache\u0027: self.cache_version_never_on})"},{"line_number":3031,"context_line":"        status, headers, body \u003d self.call_ov(req)"},{"line_number":3032,"context_line":"        self.assertEqual(status, \u0027200 OK\u0027)"},{"line_number":3033,"context_line":"        self.assertNotIn(\u0027x-versions-enabled\u0027, [h.lower() for h, _ in headers])"}],"source_content_type":"text/x-python","patch_set":2,"id":"285344b8_f3893b13","line":3030,"updated":"2023-07-06 18:44:26.000000000","message":"Existing style notwithstanding, I wonder if you would agree in general it often makes sense to localize stubs to the test case?\n\nCurrently, this test is the ONLY place we use this new stub.","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ad82d37106dfff5c29f8bafc7a96ede372c1f9f9","unresolved":true,"context_lines":[{"line_number":3053,"context_line":"        self.assertEqual(expected, json.loads(body))"},{"line_number":3054,"context_line":"        self.assertEqual(self.app.calls, ["},{"line_number":3055,"context_line":"            (\u0027GET\u0027, \u0027/v1/a/c?format\u003djson\u0027),"},{"line_number":3056,"context_line":"            (\u0027HEAD\u0027, self.build_versions_path()),"},{"line_number":3057,"context_line":"        ])"},{"line_number":3058,"context_line":""},{"line_number":3059,"context_line":"        # if it\u0027s in cache, we won\u0027t even get the HEAD"}],"source_content_type":"text/x-python","patch_set":2,"id":"1899db02_b904ee5c","line":3056,"updated":"2023-07-06 18:44:26.000000000","message":"whoa!  it\u0027s the get_info lookup HEAD reqeust!","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ad82d37106dfff5c29f8bafc7a96ede372c1f9f9","unresolved":true,"context_lines":[{"line_number":3060,"context_line":"        self.app._calls \u003d []"},{"line_number":3061,"context_line":"        self.cache_version_never_on.set("},{"line_number":3062,"context_line":"            get_cache_key(\u0027a\u0027, self.build_container_name(\u0027c\u0027)),"},{"line_number":3063,"context_line":"            {\u0027status\u0027: 404})"},{"line_number":3064,"context_line":"        req \u003d Request.blank("},{"line_number":3065,"context_line":"            \u0027/v1/a/c?versions\u0027,"},{"line_number":3066,"context_line":"            environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027GET\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7e43de4a_54ffd94e","line":3063,"updated":"2023-07-06 18:44:26.000000000","message":"Are you sure the previous request didn\u0027t already set this?  I feel like this makes our behavior assertion weaker in some real sense.","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7551543b41f9f2e6d3ce373fc0a5ec88a7002fca","unresolved":true,"context_lines":[{"line_number":3060,"context_line":"        self.app._calls \u003d []"},{"line_number":3061,"context_line":"        self.cache_version_never_on.set("},{"line_number":3062,"context_line":"            get_cache_key(\u0027a\u0027, self.build_container_name(\u0027c\u0027)),"},{"line_number":3063,"context_line":"            {\u0027status\u0027: 404})"},{"line_number":3064,"context_line":"        req \u003d Request.blank("},{"line_number":3065,"context_line":"            \u0027/v1/a/c?versions\u0027,"},{"line_number":3066,"context_line":"            environ\u003d{\u0027REQUEST_METHOD\u0027: \u0027GET\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"4bc57005_55052f63","line":3063,"in_reply_to":"7e43de4a_54ffd94e","updated":"2023-07-06 19:30:40.000000000","message":"I mean, if we want to _make sure_ we cached the HEAD, we should probably make an assertion on `self.cache_version_never_on.get(get_cache_key(...))` before we ever get to the second request -- but I generally think we\u0027ve got enough test coverage of `get_container_info` already.","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"ad82d37106dfff5c29f8bafc7a96ede372c1f9f9","unresolved":true,"context_lines":[{"line_number":3069,"context_line":"        self.assertEqual(status, \u0027200 OK\u0027)"},{"line_number":3070,"context_line":"        self.assertNotIn(\u0027x-versions-enabled\u0027, [h.lower() for h, _ in headers])"},{"line_number":3071,"context_line":"        self.assertEqual(expected, json.loads(body))"},{"line_number":3072,"context_line":"        self.assertEqual(self.app.calls, [(\u0027GET\u0027, \u0027/v1/a/c?format\u003djson\u0027)])"},{"line_number":3073,"context_line":""},{"line_number":3074,"context_line":"    def test_bytes_count(self):"},{"line_number":3075,"context_line":"        self.app.register("}],"source_content_type":"text/x-python","patch_set":2,"id":"eda14de8_0bdbc069","line":3072,"updated":"2023-07-06 18:44:26.000000000","message":"whoa, see you later useless GET\u003d\u003e404 response.  boom.  cache FTW","commit_id":"14a227f305a9e498fab761672bd8652bba8de5d2"}]}