)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"95e7d4af_d676348a","updated":"2025-11-05 01:34:58.000000000","message":"yes, probably an improvement.","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"21da4d97c7ca3a4d558db343ace7accbb3a2a746","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"4208ef93_221e1170","updated":"2025-11-06 06:37:23.000000000","message":"Just having a bit of a look at things. In the sharded db_state I do worry that the ref \u003d\u003d broker[0] ref might cause us issues.. but it\u0027s dinner time and I haven\u0027t had a chance to double check yet.\n\nThis is looking pretty good. Looking at what we do in set_sharding_state, we seem to go out of our way to make the freshdb an exact copy (kinda vacuumed) of the retiring.. copying incoming/outgoing syncs, metadata and even making the ROW_ID match. Yet we\u0027re changing the db id (ref)... maybe we\u0027re suppose to copy that too, otherwise whats the point of matching ROW_IDs, because usync will fail because the new db id wont be in any incoming or outgoing sync.\n\nSo is another way of solving some of this, just make the retiring and the fresh have the same db ID. I mean there the same ID from a replication POV, and on the same device.","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"88b3822c47a4899b5a009df27527a5d66a86a9ee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"376f9a40_e0a9593c","updated":"2025-11-05 15:12:01.000000000","message":"recheck","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"5b143888f73dfeb736e6b57870dc30aafed8bd52","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"93efa720_f03d2ada","updated":"2025-11-05 18:41:53.000000000","message":"recheck","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6fabcfd40a2e301294d220517597459e32936282","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"b0a6c750_30100025","updated":"2025-11-05 16:44:52.000000000","message":"recheck","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"639a6c08_433e13e5","updated":"2025-11-05 22:43:05.000000000","message":"so i\u0027m pretty confident initializing the state of the context correctly in load out of the get go is better than doing it ad-hoc inside the sharder; but I\u0027m realizing I could be convinced that doing it in the start method would also be perhaps justifiable.\n\nDoing it *outside* of the class that\u0027s supposed to be encapsulating all this state from us would NOT be ok IMHO.\n\nHowever, I\u0027m less confident in retrospect that the additional refactoring of the load method to fix the weird/brutal initialization style belongs in the change that adds the new fresh_ref attribute.\n\nOn one hand - yes, we should try to make confusing code more obvious.  On the other hand - yes, we should try to make diffs that are as targeted as possible.\n\nIf I was *really* committed to cleaning up load I\u0027d probably consider doing it as a prefactor and letting the new \"create vs pre-existing\" handling of fresh_db slide right in.\n\nIf I wasn\u0027t feeling so committed I\u0027d do simplest thing that could work:\n\n```\ndiff --git a/swift/container/sharder.py b/swift/container/sharder.py\nindex fb22f22ae..f7c58a6e8 100644\n--- a/swift/container/sharder.py\n+++ b/swift/container/sharder.py\n@@ -709,6 +709,8 @@ class CleavingContext(object):\n         data \u003d json.loads(data) if data else {}\n         data[\u0027ref\u0027] \u003d ref\n         data[\u0027max_row\u0027] \u003d brokers[0].get_max_row()\n+        if \u0027fresh_ref\u0027 not in data:\n+            data[\u0027fresh_ref\u0027] \u003d brokers[-1].get_info()[\u0027id\u0027]\n         return cls(**data)\n \n     def store(self, broker):\n```\n\nand try to get back around to the load pre-existing vs create rabbit hole when there\u0027s time/appetite to review and polish it.\n\nThe use-case for fresh_ref seems like a win - the logic to find and update the context stats in recon seems better IMHO.  Aside from my worry that I shouldn\u0027t have encouraged you to go re-factoring the whole damn structure of the load method just to add some new business logic to it - I think this is quite good.","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"}],"swift/container/sharder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":712,"context_line":"        :return: An instance of :class:`CleavingContext`."},{"line_number":713,"context_line":"        \"\"\""},{"line_number":714,"context_line":"        brokers \u003d broker.get_brokers()"},{"line_number":715,"context_line":"        ref \u003d cls._make_ref(brokers[0])"},{"line_number":716,"context_line":"        data \u003d brokers[-1].get_sharding_sysmeta(\u0027Context-\u0027 + ref)"},{"line_number":717,"context_line":"        data \u003d json.loads(data) if data else {}"},{"line_number":718,"context_line":"        data[\u0027ref\u0027] \u003d ref"}],"source_content_type":"text/x-python","patch_set":2,"id":"e16563b6_f4a984ef","line":715,"updated":"2025-11-05 01:34:58.000000000","message":"I think it\u0027d be better if the fresh_ref used this same _make_ref helper","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":false,"context_lines":[{"line_number":712,"context_line":"        :return: An instance of :class:`CleavingContext`."},{"line_number":713,"context_line":"        \"\"\""},{"line_number":714,"context_line":"        brokers \u003d broker.get_brokers()"},{"line_number":715,"context_line":"        ref \u003d cls._make_ref(brokers[0])"},{"line_number":716,"context_line":"        data \u003d brokers[-1].get_sharding_sysmeta(\u0027Context-\u0027 + ref)"},{"line_number":717,"context_line":"        data \u003d json.loads(data) if data else {}"},{"line_number":718,"context_line":"        data[\u0027ref\u0027] \u003d ref"}],"source_content_type":"text/x-python","patch_set":2,"id":"f12faf36_ec019f5a","line":715,"in_reply_to":"e16563b6_f4a984ef","updated":"2025-11-05 13:49:30.000000000","message":"Done","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":716,"context_line":"        data \u003d brokers[-1].get_sharding_sysmeta(\u0027Context-\u0027 + ref)"},{"line_number":717,"context_line":"        data \u003d json.loads(data) if data else {}"},{"line_number":718,"context_line":"        data[\u0027ref\u0027] \u003d ref"},{"line_number":719,"context_line":"        data[\u0027max_row\u0027] \u003d brokers[0].get_max_row()"},{"line_number":720,"context_line":"        return cls(**data)"},{"line_number":721,"context_line":""},{"line_number":722,"context_line":"    def store(self, broker):"}],"source_content_type":"text/x-python","patch_set":2,"id":"68e7ca7a_bca32710","line":719,"updated":"2025-11-05 01:34:58.000000000","message":"to me this is a *little* confusing\n\nyes it\u0027s true that while sharding the retiring_db (brokers[0]) won\u0027t have it\u0027s row\u0027s or id change - if the json.loads(data) found existing data, and discovered a *different* value for either the ref or max_row key (!?) overwriting it seems like it would just be hiding a potential bug.\n\nProbably better to do handle create/load explicitly:\n\n```\nif raw_json:\n    data \u003d json.loads()\n    # sanity/validate/backwards compat missing keys\n    if \u0027ref\u0027 in data and ref !\u003d data[\u0027ref\u0027]:\n        ...\n    if \u0027max_row\u0027 not in data:\n        data[\u0027max_row\u0027] \u003d get_max_row()\n    if \u0027fresh_ref\u0027 not in data:\n        # for a long time we didn\u0027t write this down; we do now!\n        data[\u0027fresh_ref\u0027] \u003d fresh_ref\nelse:\n    # this is a brand new cleaving context!  Make it awesome!\n    data \u003d {\n         \u0027ref\u0027: ref,\n         \u0027fresh_ref\u0027: fresh_ref,\n         \u0027max_row\u0027: get_max_row(),\n    }\nreturn data\n```\n\n... but I suppose someone could argue the prefer the more brutal style; I just think for the case when `brokers[0] \u003d\u003d brokers[-1]` we have to think harder about:\n\n```\ndata[\u0027fresh_ref\u0027] \u003d cls._make_ref(brokers[-1])\n```","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":true,"context_lines":[{"line_number":716,"context_line":"        data \u003d brokers[-1].get_sharding_sysmeta(\u0027Context-\u0027 + ref)"},{"line_number":717,"context_line":"        data \u003d json.loads(data) if data else {}"},{"line_number":718,"context_line":"        data[\u0027ref\u0027] \u003d ref"},{"line_number":719,"context_line":"        data[\u0027max_row\u0027] \u003d brokers[0].get_max_row()"},{"line_number":720,"context_line":"        return cls(**data)"},{"line_number":721,"context_line":""},{"line_number":722,"context_line":"    def store(self, broker):"}],"source_content_type":"text/x-python","patch_set":2,"id":"6ac96117_951626f6","line":719,"in_reply_to":"68e7ca7a_bca32710","updated":"2025-11-05 13:49:30.000000000","message":"Good suggestion. I added a comparison between the latest and current max_row that may be in the JSON data since according to test_cleave_repeated + test_complete_sharding + some others that failed, it\u0027s possible for new objs to get merged into retiring db and change what max_row should be","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":false,"context_lines":[{"line_number":716,"context_line":"        data \u003d brokers[-1].get_sharding_sysmeta(\u0027Context-\u0027 + ref)"},{"line_number":717,"context_line":"        data \u003d json.loads(data) if data else {}"},{"line_number":718,"context_line":"        data[\u0027ref\u0027] \u003d ref"},{"line_number":719,"context_line":"        data[\u0027max_row\u0027] \u003d brokers[0].get_max_row()"},{"line_number":720,"context_line":"        return cls(**data)"},{"line_number":721,"context_line":""},{"line_number":722,"context_line":"    def store(self, broker):"}],"source_content_type":"text/x-python","patch_set":2,"id":"f8e48a3f_313acba7","line":719,"in_reply_to":"6ac96117_951626f6","updated":"2025-11-05 22:43:05.000000000","message":"\u003e it\u0027s possible for new objs to get merged into retiring db and change what max_row should be\n\nthis is shocking and confusing to me!  once db_state SHARDED (i.e. as soon as we *start* shard*ing*) we shouldn\u0027t merge any rows into the retiring database anymore.  It\u0027s *retired* give the poor thing some rest!\n\nMy initial expectation is that those tests are just poor stubs/fakes for what happens in a real system.  I think it\u0027d be worth looking into, but probably orthogonal to this change.  i.e. a reviewer should -1 this change \"you should make max_row updates conditional\" if doing so would cause a bunch of tests to fail until they themselves tried to fix those tests.","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":1028,"context_line":"        if db_state \u003d\u003d SHARDED:"},{"line_number":1029,"context_line":"            contexts \u003d CleavingContext.load_all(broker)"},{"line_number":1030,"context_line":"            if not contexts:"},{"line_number":1031,"context_line":"                return"},{"line_number":1032,"context_line":"            context_ts \u003d max(float(ts) for c, ts in contexts)"},{"line_number":1033,"context_line":"            if context_ts + self.recon_sharded_timeout \\"},{"line_number":1034,"context_line":"                    \u003c float(Timestamp.now()):"}],"source_content_type":"text/x-python","patch_set":2,"id":"67abc483_2bd74fc1","line":1031,"updated":"2025-11-05 01:34:58.000000000","message":"I suppose it has to be possible for a db_state \u003d SHARDED to not have a CleavingContext (yet) - but is probably indicating there was some kind of error/failure in process_broker and all the OTHER stats down below seem like they might be useful/reasonable to report.","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":false,"context_lines":[{"line_number":1028,"context_line":"        if db_state \u003d\u003d SHARDED:"},{"line_number":1029,"context_line":"            contexts \u003d CleavingContext.load_all(broker)"},{"line_number":1030,"context_line":"            if not contexts:"},{"line_number":1031,"context_line":"                return"},{"line_number":1032,"context_line":"            context_ts \u003d max(float(ts) for c, ts in contexts)"},{"line_number":1033,"context_line":"            if context_ts + self.recon_sharded_timeout \\"},{"line_number":1034,"context_line":"                    \u003c float(Timestamp.now()):"}],"source_content_type":"text/x-python","patch_set":2,"id":"28073538_681f7fa7","line":1031,"in_reply_to":"67abc483_2bd74fc1","updated":"2025-11-05 13:49:30.000000000","message":"removed the early return bit","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":1034,"context_line":"                    \u003c float(Timestamp.now()):"},{"line_number":1035,"context_line":"                # last context timestamp too old for the"},{"line_number":1036,"context_line":"                # broker to be recorded"},{"line_number":1037,"context_line":"                return"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"            fresh_local_id \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":1040,"context_line":"            matched_contexts \u003d [(c, ts) for c, ts in contexts"}],"source_content_type":"text/x-python","patch_set":2,"id":"229c1d10_8fbaedb0","line":1037,"updated":"2025-11-05 01:34:58.000000000","message":"I think this is pulling the same trick with \"could be grabbing the wrong context\" - but maybe it\u0027s justified for nodeA to keep reporting it\u0027s shared/cleaved database as \"in-progress\" as long as nodeB is still updating/working on it\u0027s cleave context tho.\n\nStill feels like we\u0027re searching the context multiple times when we could do it in a loop.\n\n```\nlocal_context \u003d None\nfound_recent \u003d False\nfor c, ts in contexts:\n    if ts \u003c cutoff:\n        found_recent \u003d True\n    if c.fresh_ref \u003d local_id:\n        if local_context:\n            logging.wtf()\n        local_context \u003d c\nif not found_recent:\n    # no stats for u!\n    return\nif local_context:\n    info.update(new_hawtness)\n```","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":true,"context_lines":[{"line_number":1034,"context_line":"                    \u003c float(Timestamp.now()):"},{"line_number":1035,"context_line":"                # last context timestamp too old for the"},{"line_number":1036,"context_line":"                # broker to be recorded"},{"line_number":1037,"context_line":"                return"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"            fresh_local_id \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":1040,"context_line":"            matched_contexts \u003d [(c, ts) for c, ts in contexts"}],"source_content_type":"text/x-python","patch_set":2,"id":"353819d7_51ea918a","line":1037,"in_reply_to":"229c1d10_8fbaedb0","updated":"2025-11-05 13:49:30.000000000","message":"Solid structure!, I took out the extra check for the duplicate local context because that shouldn\u0027t? happen most likely","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":1034,"context_line":"                    \u003c float(Timestamp.now()):"},{"line_number":1035,"context_line":"                # last context timestamp too old for the"},{"line_number":1036,"context_line":"                # broker to be recorded"},{"line_number":1037,"context_line":"                return"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"            fresh_local_id \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":1040,"context_line":"            matched_contexts \u003d [(c, ts) for c, ts in contexts"}],"source_content_type":"text/x-python","patch_set":2,"id":"9b8acc10_d752ab69","line":1037,"in_reply_to":"353819d7_51ea918a","updated":"2025-11-05 22:43:05.000000000","message":"agree\u0027d and unless you want to write a test for it better to not have the code - since the best we could come up to DO with is `logging.wtf()` anyway.\n\nThe combined loop sort of obfuscates the fact that if there WAS duplicate c.fresh_ref we\u0027d just pick one at random.\n\nwe could *probably* organize the loop to early exit by ordering the context by the their ts and picking the \"first\" one:\n\n```\n\u003e\u003e\u003e from operator import itemgetter\n\u003e\u003e\u003e a \u003d [(\u0027foo\u0027, 1), (\u0027bar\u0027, 0), (\u0027baz\u0027, 3)]\n\u003e\u003e\u003e sorted(a, key\u003ditemgetter(1))\n[(\u0027bar\u0027, 0), (\u0027foo\u0027, 1), (\u0027baz\u0027, 3)]\n```\n\nprobably `reverse\u003dTrue` to find the most recent context first?","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":true,"context_lines":[{"line_number":1034,"context_line":"                    \u003c float(Timestamp.now()):"},{"line_number":1035,"context_line":"                # last context timestamp too old for the"},{"line_number":1036,"context_line":"                # broker to be recorded"},{"line_number":1037,"context_line":"                return"},{"line_number":1038,"context_line":""},{"line_number":1039,"context_line":"            fresh_local_id \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":1040,"context_line":"            matched_contexts \u003d [(c, ts) for c, ts in contexts"}],"source_content_type":"text/x-python","patch_set":2,"id":"bd849754_4ab571f2","line":1037,"in_reply_to":"9b8acc10_d752ab69","updated":"2025-11-07 20:25:29.000000000","message":"I think technically if there was a duplicate fresh_ref it\u0027d pick whichever was the newest anyway at the end of the loop since (unless I\u0027m reading things REALLY wrong) contexts should already by in order by timestamp. But for a higher guarantee I\u0027ve added a sort + early exit","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":1041,"context_line":"                                if c.fresh_ref \u003d\u003d fresh_local_id]"},{"line_number":1042,"context_line":"            if matched_contexts:"},{"line_number":1043,"context_line":"                sharded_context \u003d max(matched_contexts,"},{"line_number":1044,"context_line":"                                      key\u003dlambda ctx_ts: float(ctx_ts[1]))[0]"},{"line_number":1045,"context_line":""},{"line_number":1046,"context_line":"        update_own_shard_range_stats(broker, own_shard_range)"},{"line_number":1047,"context_line":"        info \u003d self._make_stats_info(broker, node, own_shard_range)"}],"source_content_type":"text/x-python","patch_set":2,"id":"4d0fe6ff_3db88a84","line":1044,"updated":"2025-11-05 01:34:58.000000000","message":"I feel like I\u0027d rather disambiguate the search/select like:\n\n```\n# we expect only the one\ntry:\n    (c, ts), \u003d matched_contexts\nexcept ValueError:\n    self.logger.error(\u0027wtf is there multiple context\u0027\n                      \u0027for this fresh_db!? %r\u0027, matched_context)\n```\n\n... but this method is already so messy, i\u0027m not sure which states/conditions warrant early exit vs anemic results.","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":true,"context_lines":[{"line_number":1041,"context_line":"                                if c.fresh_ref \u003d\u003d fresh_local_id]"},{"line_number":1042,"context_line":"            if matched_contexts:"},{"line_number":1043,"context_line":"                sharded_context \u003d max(matched_contexts,"},{"line_number":1044,"context_line":"                                      key\u003dlambda ctx_ts: float(ctx_ts[1]))[0]"},{"line_number":1045,"context_line":""},{"line_number":1046,"context_line":"        update_own_shard_range_stats(broker, own_shard_range)"},{"line_number":1047,"context_line":"        info \u003d self._make_stats_info(broker, node, own_shard_range)"}],"source_content_type":"text/x-python","patch_set":2,"id":"c2ab81dc_19ead260","line":1044,"in_reply_to":"4d0fe6ff_3db88a84","updated":"2025-11-05 13:49:30.000000000","message":"Realistically the max_ts check for the local context thing isn\u0027t needed, I was just being cautious at the time. I feel the method\u0027s much cleaner with the combined loop now!","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":false,"context_lines":[{"line_number":1041,"context_line":"                                if c.fresh_ref \u003d\u003d fresh_local_id]"},{"line_number":1042,"context_line":"            if matched_contexts:"},{"line_number":1043,"context_line":"                sharded_context \u003d max(matched_contexts,"},{"line_number":1044,"context_line":"                                      key\u003dlambda ctx_ts: float(ctx_ts[1]))[0]"},{"line_number":1045,"context_line":""},{"line_number":1046,"context_line":"        update_own_shard_range_stats(broker, own_shard_range)"},{"line_number":1047,"context_line":"        info \u003d self._make_stats_info(broker, node, own_shard_range)"}],"source_content_type":"text/x-python","patch_set":2,"id":"43138802_ccba9573","line":1044,"in_reply_to":"c2ab81dc_19ead260","updated":"2025-11-05 22:43:05.000000000","message":"Done","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":1048,"context_line":""},{"line_number":1049,"context_line":"        if sharded_context:"},{"line_number":1050,"context_line":"            info[\"total_replicate_calls\"] \u003d sharded_context.replication_count"},{"line_number":1051,"context_line":"            info[\"total_replicate_time\"] \u003d sharded_context.replication_time"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"        if processing_time:"},{"line_number":1054,"context_line":"            info[\u0027processing_time\u0027] \u003d processing_time"}],"source_content_type":"text/x-python","patch_set":2,"id":"2de74e57_9ec853dd","line":1051,"updated":"2025-11-05 01:34:58.000000000","message":"maybe better to move this all under `db_state \u003d\u003d SHARDED` so we don\u0027t have to have multiple if blocks to handle the same condition.","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":false,"context_lines":[{"line_number":1048,"context_line":""},{"line_number":1049,"context_line":"        if sharded_context:"},{"line_number":1050,"context_line":"            info[\"total_replicate_calls\"] \u003d sharded_context.replication_count"},{"line_number":1051,"context_line":"            info[\"total_replicate_time\"] \u003d sharded_context.replication_time"},{"line_number":1052,"context_line":""},{"line_number":1053,"context_line":"        if processing_time:"},{"line_number":1054,"context_line":"            info[\u0027processing_time\u0027] \u003d processing_time"}],"source_content_type":"text/x-python","patch_set":2,"id":"75d5005f_31cc2a5c","line":1051,"in_reply_to":"2de74e57_9ec853dd","updated":"2025-11-05 13:49:30.000000000","message":"Done","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":2154,"context_line":"                broker, \u0027Passing over already sharded container\u0027)"},{"line_number":2155,"context_line":"            return True"},{"line_number":2156,"context_line":""},{"line_number":2157,"context_line":"        cleaving_context \u003d CleavingContext.load(broker)"},{"line_number":2158,"context_line":"        if not cleaving_context.misplaced_done:"},{"line_number":2159,"context_line":"            # ensure any misplaced objects in the source broker are moved; note"},{"line_number":2160,"context_line":"            # that this invocation of _move_misplaced_objects is targetted at"}],"source_content_type":"text/x-python","patch_set":2,"id":"e877fc68_63129bd6","line":2157,"updated":"2025-11-05 01:34:58.000000000","message":"I think this is where a sharding db will \"load or create\" the CleavingContext - this would be the best time to set the fresh_ref (probably encapsulated in the \"create\" path of the load method)","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":false,"context_lines":[{"line_number":2154,"context_line":"                broker, \u0027Passing over already sharded container\u0027)"},{"line_number":2155,"context_line":"            return True"},{"line_number":2156,"context_line":""},{"line_number":2157,"context_line":"        cleaving_context \u003d CleavingContext.load(broker)"},{"line_number":2158,"context_line":"        if not cleaving_context.misplaced_done:"},{"line_number":2159,"context_line":"            # ensure any misplaced objects in the source broker are moved; note"},{"line_number":2160,"context_line":"            # that this invocation of _move_misplaced_objects is targetted at"}],"source_content_type":"text/x-python","patch_set":2,"id":"f2523710_e7b86711","line":2157,"in_reply_to":"e877fc68_63129bd6","updated":"2025-11-05 13:49:30.000000000","message":"Done","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"04433c7c0a6571661605c1361ac40323737535fc","unresolved":true,"context_lines":[{"line_number":2192,"context_line":"        else:"},{"line_number":2193,"context_line":"            cleaving_context.start()"},{"line_number":2194,"context_line":"            own_shard_range \u003d broker.get_own_shard_range()"},{"line_number":2195,"context_line":"            cleaving_context.fresh_ref \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":2196,"context_line":"            cleaving_context.cursor \u003d own_shard_range.lower_str"},{"line_number":2197,"context_line":"            cleaving_context.ranges_todo \u003d len(ranges_todo)"},{"line_number":2198,"context_line":"            self.db_logger.info("}],"source_content_type":"text/x-python","patch_set":2,"id":"38f94ed7_f1797a11","line":2195,"updated":"2025-11-05 01:34:58.000000000","message":"I don\u0027t understand why this is the correct place to add a value into the fresh_ref attribute.","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":true,"context_lines":[{"line_number":2192,"context_line":"        else:"},{"line_number":2193,"context_line":"            cleaving_context.start()"},{"line_number":2194,"context_line":"            own_shard_range \u003d broker.get_own_shard_range()"},{"line_number":2195,"context_line":"            cleaving_context.fresh_ref \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":2196,"context_line":"            cleaving_context.cursor \u003d own_shard_range.lower_str"},{"line_number":2197,"context_line":"            cleaving_context.ranges_todo \u003d len(ranges_todo)"},{"line_number":2198,"context_line":"            self.db_logger.info("}],"source_content_type":"text/x-python","patch_set":2,"id":"81264fc9_1dc13e8c","line":2195,"in_reply_to":"0a3016ea_2758e186","updated":"2025-11-07 20:25:29.000000000","message":"Good catch, + since that was the only real place we used the function I thought it reasonable to clear out the extra code","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"6f57e010a705788ffe9fcd6713330d0b68131a40","unresolved":true,"context_lines":[{"line_number":2192,"context_line":"        else:"},{"line_number":2193,"context_line":"            cleaving_context.start()"},{"line_number":2194,"context_line":"            own_shard_range \u003d broker.get_own_shard_range()"},{"line_number":2195,"context_line":"            cleaving_context.fresh_ref \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":2196,"context_line":"            cleaving_context.cursor \u003d own_shard_range.lower_str"},{"line_number":2197,"context_line":"            cleaving_context.ranges_todo \u003d len(ranges_todo)"},{"line_number":2198,"context_line":"            self.db_logger.info("}],"source_content_type":"text/x-python","patch_set":2,"id":"b20ebdad_131c21f5","line":2195,"in_reply_to":"38f94ed7_f1797a11","updated":"2025-11-05 13:49:30.000000000","message":"I figured it was right since that\u0027s when start() gets called for the first+only time and probably where the fresh_db ref would be most \"correct\" I guess","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":2192,"context_line":"        else:"},{"line_number":2193,"context_line":"            cleaving_context.start()"},{"line_number":2194,"context_line":"            own_shard_range \u003d broker.get_own_shard_range()"},{"line_number":2195,"context_line":"            cleaving_context.fresh_ref \u003d broker.get_info()[\u0027id\u0027]"},{"line_number":2196,"context_line":"            cleaving_context.cursor \u003d own_shard_range.lower_str"},{"line_number":2197,"context_line":"            cleaving_context.ranges_todo \u003d len(ranges_todo)"},{"line_number":2198,"context_line":"            self.db_logger.info("}],"source_content_type":"text/x-python","patch_set":2,"id":"0a3016ea_2758e186","line":2195,"in_reply_to":"b20ebdad_131c21f5","updated":"2025-11-05 22:43:05.000000000","message":"ah, \"right after start\" is reasonable!!!  So why\u0027d you do it \"the line after right after start\" 😊\n\nWTH does start do, anyway?  😄\n\n```\n    def start(self):\n        self.cursor \u003d \u0027\u0027\n        self.ranges_done \u003d 0\n        self.ranges_todo \u003d 0\n        self.cleaving_done \u003d False\n        self.cleave_to_row \u003d self.max_row\n        self.replication_count \u003d 0\n        self.replication_time \u003d 0\n```\n\ncompared to `__init__`\n\n```\n    def __init__(self, ref, cursor\u003d\u0027\u0027, max_row\u003dNone, cleave_to_row\u003dNone,\n                 last_cleave_to_row\u003dNone, cleaving_done\u003dFalse,\n                 misplaced_done\u003dFalse, ranges_done\u003d0, ranges_todo\u003d0,\n                 replication_count\u003d0, replication_time\u003d0, fresh_ref\u003dNone):\n        self.ref \u003d ref\n        self._cursor \u003d None\n        self.cursor \u003d cursor\n        self.max_row \u003d max_row\n        self.cleave_to_row \u003d cleave_to_row\n        self.last_cleave_to_row \u003d last_cleave_to_row\n        self.cleaving_done \u003d cleaving_done\n        self.misplaced_done \u003d misplaced_done\n        self.ranges_done \u003d ranges_done\n        self.ranges_todo \u003d ranges_todo\n        self.replication_count \u003d replication_count\n        self.replication_time \u003d replication_time\n        self.fresh_ref \u003d fresh_ref\n```\n\nI feel like start is sort of a dumb method.\n\n```\n@@ -2212,7 +2218,7 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator):\n                 cleaving_context.ranges_done,\n                 cleaving_context.ranges_todo)\n         else:\n-            cleaving_context.start()\n+            cleaving_context.cleave_to_row \u003d cleaving_context.max_row\n             own_shard_range \u003d broker.get_own_shard_range()\n             cleaving_context.cursor \u003d own_shard_range.lower_str\n             cleaving_context.ranges_todo \u003d len(ranges_todo)\n```\n\n^ tests pass\n\nSo apparently `self.cleave_to_row \u003d self.max_row` is some dark magic?\n\nMaybe we should just *initialize* `cleave_to_row \u003d max_row` ??? why are they ever different!?  Is this the heart of the mystery as to how retiring db\u0027s EVER grow new rows!?","commit_id":"dbab71d9843758ffde3c9f3f03d6ddf96072af4e"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":725,"context_line":"            if \u0027ref\u0027 in data and data[\u0027ref\u0027] !\u003d ref:"},{"line_number":726,"context_line":"                raise ValueError("},{"line_number":727,"context_line":"                    \u0027Unexpected ref in stored context: expected %s, got %s\u0027 %"},{"line_number":728,"context_line":"                    (ref, data[\u0027ref\u0027]))"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"}],"source_content_type":"text/x-python","patch_set":3,"id":"1d4ea533_0aeab4f6","line":728,"updated":"2025-11-05 22:43:05.000000000","message":"I\u0027m not 100% where this ValueError would be caught and so I immediately don\u0027t like it.\n\nwhen I remove this check/condition \u0026 and is associated `data[\u0027ref\u0027] \u003d ref` in L733 I get exactly one test failure in `test.unit.container.test_sharder`:\n\n```\nself \u003d \u003ctest.unit.container.test_sharder.TestCleavingContext testMethod\u003dtest_load_with_mismatched_ref\u003e\n\n    def test_load_with_mismatched_ref(self):\n        broker \u003d self._make_broker()\n        db_id \u003d broker.get_info()[\u0027id\u0027]\n        mismatched_ref \u003d \u0027wrong-ref-12345\u0027\n    \n        params \u003d {\u0027ref\u0027: mismatched_ref,\n                  \u0027cursor\u0027: \u0027curs\u0027,\n                  \u0027max_row\u0027: 2,\n                  \u0027cleave_to_row\u0027: 2}\n        key \u003d \u0027X-Container-Sysmeta-Shard-Context-%s\u0027 % db_id\n        broker.update_metadata(\n            {key: (json.dumps(params), Timestamp.now().internal)})\n    \n\u003e       with self.assertRaises(ValueError) as cm:\nE       AssertionError: ValueError not raised\n\nswift/test/unit/container/test_sharder.py:8344: AssertionError\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d short test summary info \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\nFAILED swift/test/unit/container/test_sharder.py::TestCleavingContext::test_load_with_mismatched_ref - AssertionError: ValueError not raised\n```\n\n... which certainly enshrines the behavior as expected from the POV of the interface - but doesn\u0027t really describe how we expect the sharder to deal with it.\n\nIMHO the new `TestCleavingContext::test_load_with_mismatched_ref` is sufficiently contrived that it could be removed and a potentially better implementation might omit this check entirely.\n\nHOWEVER, since you already have the test - and `data[\u0027ref\u0027] !\u003d ref` would probably just be a bug somewhere else; it\u0027s not *un-reasonable* to validate your assumptions - ESPECIALLY if we\u0027re going to remove the `data[\u0027ref\u0027] \u003d ref` change!  Which I support, because it\u0027s ALWAYS been a consistent part of this structure:\n\n565748: Add sharder daemon, manage_shard_ranges tool and probe tests | https://review.opendev.org/c/openstack/swift/+/565748\n\nAlthough, the db_id itself has changed since we started using it to track the cleaving context, which is interesting and suggests to me that upgrade probably lost and reset any in-progress cleaving:\n\n828069: DB: Encode the device to the DB id | https://review.opendev.org/c/openstack/swift/+/828069\n\nIn summary, I suggest you ditch the `if ref in data` and go ahead with your bold bold assertion:\n\n```\ndiff --git a/swift/container/sharder.py b/swift/container/sharder.py\nindex d6dc135b3..102336ae6 100644\n--- a/swift/container/sharder.py\n+++ b/swift/container/sharder.py\n@@ -722,15 +722,20 @@ class CleavingContext(object):\n             data \u003d json.loads(raw_data)\n \n             # ref should match the retiring DB\n-            if \u0027ref\u0027 in data and data[\u0027ref\u0027] !\u003d ref:\n+            if data[\u0027ref\u0027] !\u003d ref:\n                 raise ValueError(\n                     \u0027Unexpected ref in stored context: expected %s, got %s\u0027 %\n                     (ref, data[\u0027ref\u0027]))\n \n-            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:\n+            if data[\u0027max_row\u0027] \u003c max_row:\n+                # TODO make this a factory method so we can re-use the deamon\u0027s logger\n+                import logging\n+                logging.debug(\n+                    \u0027Unexpected increasing max_row detected\u0027\n+                    \u0027 in retiring database %s\u003d\u003e%s\u0027, data[\u0027max_row\u0027], max_row)\n+                # this maintains previous behavior and keeps tests happy, but is not well understood\n                 data[\u0027max_row\u0027] \u003d max_row\n \n-            data[\u0027ref\u0027] \u003d ref\n             # this is new so set it for backwards compat\n             if \u0027fresh_ref\u0027 not in data:\n                 data[\u0027fresh_ref\u0027] \u003d fresh_ref\n```\n\nWith the justification that it should never happen, but if it did our code would explode LOUDLY (somewhere?) and if our assumptions are wrong it\u0027d be better to know it than not!","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"21da4d97c7ca3a4d558db343ace7accbb3a2a746","unresolved":true,"context_lines":[{"line_number":725,"context_line":"            if \u0027ref\u0027 in data and data[\u0027ref\u0027] !\u003d ref:"},{"line_number":726,"context_line":"                raise ValueError("},{"line_number":727,"context_line":"                    \u0027Unexpected ref in stored context: expected %s, got %s\u0027 %"},{"line_number":728,"context_line":"                    (ref, data[\u0027ref\u0027]))"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"}],"source_content_type":"text/x-python","patch_set":3,"id":"c8c5386c_bcf9e797","line":728,"in_reply_to":"1d4ea533_0aeab4f6","updated":"2025-11-06 06:37:23.000000000","message":"CleaveContext.load_all I believe will check for a value error and skip that \"context\". But yeah, not sure of anything else that\u0027ll deal with a ValueError.\n\nBut related to that I worry that on line 725 we have:\n```\nif \u0027ref\u0027 in data and data[\u0027ref\u0027] !\u003d ref:\n```\n\nWhich will then raise a ValueError and ref is defined as `ref \u003d cls._make_ref(brokers[0])`\nBUT when we\u0027re in a SHARDED db state, there is only 1 DB, so brokers[0] will be the fresh db. And therefore are we value erroring out the context that might be the one we want.\n\nInterestingly we go to alot of effort in to_sharding_state to make the freshdb a copy of the retiring one, we even go out of our way to keep the max_row_id the same.. by why bother doing that if we\u0027re changing the db id (ref)? Which I just confirmed in my saio:\n\n```\nIn [12]: broker.get_brokers()[0].get_info()[\u0027id\u0027]\nOut[12]: \u0027908ab534-2b8c-434a-afe0-113e6343da97-sdb1\u0027\n\nIn [13]: broker.get_brokers()[1].get_info()[\u0027id\u0027]\nOut[13]: \u0027117d7df9-7193-4d26-a9b0-d1ba29d3ea90-\u0027\n\nIn [14]: broker.get_brokers()[0].get_max_row()\nOut[14]: 260\n\nIn [15]: broker.get_brokers()[1].get_max_row()\nOut[15]: 260\n```\nNOTE: it\u0027s also annoying that we don\u0027t get the device name inserted into the freshdb\u0027s id.. which is another bug I just found.\n\nAnother fix, although not sure I\u0027ve throught it through, but if the intent of building a freshdb was to in essence replace the retiring (ie vacuum it) then why don\u0027t we make the db id\u0027s the same? It seems we kinda meant to, otherwise why make the row_id the same?","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":true,"context_lines":[{"line_number":725,"context_line":"            if \u0027ref\u0027 in data and data[\u0027ref\u0027] !\u003d ref:"},{"line_number":726,"context_line":"                raise ValueError("},{"line_number":727,"context_line":"                    \u0027Unexpected ref in stored context: expected %s, got %s\u0027 %"},{"line_number":728,"context_line":"                    (ref, data[\u0027ref\u0027]))"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"}],"source_content_type":"text/x-python","patch_set":3,"id":"b2633a63_c4f8dd7a","line":728,"in_reply_to":"c8c5386c_bcf9e797","updated":"2025-11-07 20:25:29.000000000","message":"If I\u0027m properly comprehending the chats over the last 2 or so days, though setting the fresh_db\u0027s ID is a good idea WRT simplicity + finally realizing a convention that was maybe intended some years back, it would change the way we currently perceive db_ids should be handled + bring up some complex thoughts on how to handle dbs that shard \u003e shrink \u003e shard agian that we haven\u0027t fully discussed. In regards to all of that I\u0027ve kept the implementation with the fresh_ref being different from the original ref","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":728,"context_line":"                    (ref, data[\u0027ref\u0027]))"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"            data[\u0027ref\u0027] \u003d ref"},{"line_number":734,"context_line":"            # this is new so set it for backwards compat"}],"source_content_type":"text/x-python","patch_set":3,"id":"f6bf10f1_3654252a","line":731,"updated":"2025-11-05 22:43:05.000000000","message":"I think the `\u0027max_row\u0027 not IN data` condition smells wrong.\n\nThe `data[\u0027max_row\u0027] \u003c max_row` condition is *very* interesting; perhaps even worth a worth a debug message.\n\n```\n(vagrant-swift-all-in-one) cgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ git diff\ndiff --git a/swift/container/sharder.py b/swift/container/sharder.py\nindex d6dc135b3..c01cf9677 100644\n--- a/swift/container/sharder.py\n+++ b/swift/container/sharder.py\n@@ -727,8 +727,10 @@ class CleavingContext(object):\n                     \u0027Unexpected ref in stored context: expected %s, got %s\u0027 %\n                     (ref, data[\u0027ref\u0027]))\n \n-            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:\n-                data[\u0027max_row\u0027] \u003d max_row\n+            if \u0027max_row\u0027 not in data:\n+                raise Exception(\u0027missing max_row!? %r\u0027 % (data))\n+            if data[\u0027max_row\u0027] \u003c max_row:\n+                raise Exception(\u0027increasing max_row!? %r\u0027 % (data))\n \n             data[\u0027ref\u0027] \u003d ref\n             # this is new so set it for backwards compat\n```\n\n```\nFAILED swift/test/unit/container/test_sharder.py::TestSharder::test_cleave_repeated - Exception: increasing max_row!? {\u0027ref\u0027: \u002703d92ada-7415-4858-9d65-10e4ad30d861-sda\u0027, \u0027fresh_ref\u0027: \u002727fdef5d-5ed2-4f7c-b6d3-edd21548888c-mnt\u0027, \u0027cursor\u0027: \u0027\u0027, \u0027max_row\u0027: 10, \u0027cleave_to_row\u0027: 10, \u0027last_cleave_to_row\u0027: None, \u0027cleaving_done\u0027: True, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 2, \u0027ranges_...\nFAILED swift/test/unit/container/test_sharder.py::TestSharder::test_complete_sharding_root - Exception: increasing max_row!? {\u0027ref\u0027: \u0027fd74aa59-a8f9-40c8-8e45-972ee17b064b-sda\u0027, \u0027fresh_ref\u0027: \u002765bd7a28-1bb8-468d-8222-1db9aaeb0134-mnt\u0027, \u0027cursor\u0027: \u0027\u0027, \u0027max_row\u0027: 1, \u0027cleave_to_row\u0027: 1, \u0027last_cleave_to_row\u0027: None, \u0027cleaving_done\u0027: True, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 0, \u0027ranges_to...\nFAILED swift/test/unit/container/test_sharder.py::TestSharder::test_complete_sharding_shard - Exception: increasing max_row!? {\u0027ref\u0027: \u00279016adec-9bdb-4d58-b559-434803198c39-sda\u0027, \u0027fresh_ref\u0027: \u0027dc838275-584e-4883-a1b7-f20bc63be2c5-mnt\u0027, \u0027cursor\u0027: \u0027\u0027, \u0027max_row\u0027: 1, \u0027cleave_to_row\u0027: 1, \u0027last_cleave_to_row\u0027: None, \u0027cleaving_done\u0027: True, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 0, \u0027ranges_to...\nFAILED swift/test/unit/container/test_sharder.py::TestSharder::test_sharded_record_sharding_progress_missing_contexts - Exception: increasing max_row!? {\u0027ref\u0027: \u00278ecbcbac-7ed7-48ea-8140-22091a09e2cb-sda\u0027, \u0027fresh_ref\u0027: \u0027fcaf8a54-c0a6-42f0-bf68-98d159abb6e1-mnt\u0027, \u0027cursor\u0027: \u0027\u0027, \u0027max_row\u0027: 1, \u0027cleave_to_row\u0027: 1, \u0027last_cleave_to_row\u0027: None, \u0027cleaving_done\u0027: True, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 0, \u0027ranges_to...\nFAILED swift/test/unit/container/test_sharder.py::TestCleavingContext::test_load - Exception: increasing max_row!? {\u0027ref\u0027: \u00271806e43a-0c79-4b89-8460-80e5eb3bd25a-sda\u0027, \u0027cursor\u0027: \u0027curs\u0027, \u0027max_row\u0027: 2, \u0027cleave_to_row\u0027: 2, \u0027last_cleave_to_row\u0027: 1, \u0027cleaving_done\u0027: False, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 2, \u0027ranges_todo\u0027: 4, \u0027replication_count\u0027: 2, \u0027replication_time\u0027: 0.5}\nFAILED swift/test/unit/container/test_sharder.py::TestCleavingContext::test_load_without_fresh_ref - Exception: increasing max_row!? {\u0027ref\u0027: \u0027fdcd7e60-3159-47fe-909c-dd7d4a6f10a5-sda\u0027, \u0027cursor\u0027: \u0027curs\u0027, \u0027max_row\u0027: 2, \u0027cleave_to_row\u0027: 2, \u0027last_cleave_to_row\u0027: 1, \u0027cleaving_done\u0027: False, \u0027misplaced_done\u0027: True}\nFAILED swift/test/unit/container/test_sharder.py::TestCleavingContext::test_store_add_row_load - Exception: increasing max_row!? {\u0027ref\u0027: \u0027beb1b455-c016-4ead-b9c7-da74c6bbfcac-sda\u0027, \u0027fresh_ref\u0027: None, \u0027cursor\u0027: \u0027curs\u0027, \u0027max_row\u0027: 1, \u0027cleave_to_row\u0027: 1, \u0027last_cleave_to_row\u0027: 0, \u0027cleaving_done\u0027: True, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 0, \u0027ranges_todo\u0027: 0, \u0027replication_count\u0027: 0, \u0027repl...\nFAILED swift/test/unit/container/test_sharder.py::TestCleavingContext::test_store_add_row_load_old_style - Exception: increasing max_row!? {\u0027ref\u0027: \u002785e0a22b-4ece-4346-969e-d4fc80e84fac-sda\u0027, \u0027fresh_ref\u0027: None, \u0027cursor\u0027: \u0027curs\u0027, \u0027max_row\u0027: 1, \u0027cleave_to_row\u0027: 1, \u0027last_cleave_to_row\u0027: 0, \u0027cleaving_done\u0027: True, \u0027misplaced_done\u0027: True, \u0027ranges_done\u0027: 0, \u0027ranges_todo\u0027: 0, \u0027replication_count\u0027: 0, \u0027repl...\n\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d 8 failed, 170 passed, 1645 subtests passed in 25.05s \u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\n```\n\nI think test results support my intuition that the problem in only \"retiring db grows new rows (!?)\" and never \"sometimes context looses data (!!!!)\"\n\nagain, max_row has been there since the begining:\n\n```\n2641814010 (Matthew Oliver   2018-05-02 10:47:51 +0100  708)         data \u003d brokers[-1].get_sharding_sysmeta(\u0027Context-\u0027 + ref)\n2641814010 (Matthew Oliver   2018-05-02 10:47:51 +0100  709)         data \u003d json.loads(data) if data else {}\n2641814010 (Matthew Oliver   2018-05-02 10:47:51 +0100  710)         data[\u0027ref\u0027] \u003d ref\n2641814010 (Matthew Oliver   2018-05-02 10:47:51 +0100  711)         data[\u0027max_row\u0027] \u003d brokers[0].get_max_row()\n2641814010 (Matthew Oliver   2018-05-02 10:47:51 +0100  712)         return cls(**data)\n```","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":true,"context_lines":[{"line_number":728,"context_line":"                    (ref, data[\u0027ref\u0027]))"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"            data[\u0027ref\u0027] \u003d ref"},{"line_number":734,"context_line":"            # this is new so set it for backwards compat"}],"source_content_type":"text/x-python","patch_set":3,"id":"8e404a6e_14a4c854","line":731,"in_reply_to":"ce6edf0c_060083b3","updated":"2025-11-07 20:25:29.000000000","message":"Since birth I was raised to always check for keys in python dicts by reflex, took that out. ✔️","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"21da4d97c7ca3a4d558db343ace7accbb3a2a746","unresolved":true,"context_lines":[{"line_number":728,"context_line":"                    (ref, data[\u0027ref\u0027]))"},{"line_number":729,"context_line":""},{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"            data[\u0027ref\u0027] \u003d ref"},{"line_number":734,"context_line":"            # this is new so set it for backwards compat"}],"source_content_type":"text/x-python","patch_set":3,"id":"ce6edf0c_060083b3","line":731,"in_reply_to":"f6bf10f1_3654252a","updated":"2025-11-06 06:37:23.000000000","message":"Yeah, past me, I must have thought of a reason that the retired db could change. probably some eventual consistency or WAL belts and braces. I\u0027ll need to think on that.. it was 7 years ago :P","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"            data[\u0027ref\u0027] \u003d ref"},{"line_number":734,"context_line":"            # this is new so set it for backwards compat"},{"line_number":735,"context_line":"            if \u0027fresh_ref\u0027 not in data:"},{"line_number":736,"context_line":"                data[\u0027fresh_ref\u0027] \u003d fresh_ref"}],"source_content_type":"text/x-python","patch_set":3,"id":"b54bee11_ff023384","line":733,"updated":"2025-11-05 22:43:05.000000000","message":"this seems redundant given the assertion in L726","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":false,"context_lines":[{"line_number":730,"context_line":"            if \u0027max_row\u0027 not in data or data[\u0027max_row\u0027] \u003c max_row:"},{"line_number":731,"context_line":"                data[\u0027max_row\u0027] \u003d max_row"},{"line_number":732,"context_line":""},{"line_number":733,"context_line":"            data[\u0027ref\u0027] \u003d ref"},{"line_number":734,"context_line":"            # this is new so set it for backwards compat"},{"line_number":735,"context_line":"            if \u0027fresh_ref\u0027 not in data:"},{"line_number":736,"context_line":"                data[\u0027fresh_ref\u0027] \u003d fresh_ref"}],"source_content_type":"text/x-python","patch_set":3,"id":"51a33a9a_b632d6c8","line":733,"in_reply_to":"b54bee11_ff023384","updated":"2025-11-07 20:25:29.000000000","message":"Done","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":1053,"context_line":""},{"line_number":1054,"context_line":"        if db_state \u003d\u003d SHARDED:"},{"line_number":1055,"context_line":"            contexts \u003d CleavingContext.load_all(broker)"},{"line_number":1056,"context_line":"            if contexts:"},{"line_number":1057,"context_line":"                recent_ctx_exists \u003d False"},{"line_number":1058,"context_line":"                local_ctx \u003d None"},{"line_number":1059,"context_line":"                # see if we have a context recent enough to be reported,"}],"source_content_type":"text/x-python","patch_set":3,"id":"969fb014_29594bdf","line":1056,"updated":"2025-11-05 22:43:05.000000000","message":"this is quite subtle and well tested, kudos!\n\n```\nswift/test/unit/container/test_sharder.py::TestSharder::test_sharded_record_sharding_progress_missing_contexts\n```\n\nessentially we want to record *something* even if contexts is empty!  I like it!","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":1066,"context_line":"                        local_ctx \u003d c"},{"line_number":1067,"context_line":""},{"line_number":1068,"context_line":"                if not recent_ctx_exists:"},{"line_number":1069,"context_line":"                    return"},{"line_number":1070,"context_line":""},{"line_number":1071,"context_line":"                if local_ctx:"},{"line_number":1072,"context_line":"                    info[\"total_replicate_calls\"] \u003d local_ctx.replication_count"}],"source_content_type":"text/x-python","patch_set":3,"id":"6f0d116f_1107dfe5","line":1069,"updated":"2025-11-05 22:43:05.000000000","message":"this is the bit that causes the wrong behavior if we loop over the empty list (we never set recent_ctx_exists to True, because it doesn\u0027t!) - and yet without any context to tell us it\u0027s old we shouldn\u0027t return - so this guard condition is actually \"any_context_exists and all_context_are_old\"\n\nalternative approach is to fail open:\n\n```\ndiff --git a/swift/container/sharder.py b/swift/container/sharder.py\nindex d6dc135b3..69462d1ed 100644\n--- a/swift/container/sharder.py\n+++ b/swift/container/sharder.py\n@@ -1053,24 +1053,30 @@ class ContainerSharder(ContainerSharderConf, ContainerReplicator):\n \n         if db_state \u003d\u003d SHARDED:\n             contexts \u003d CleavingContext.load_all(broker)\n-            if contexts:\n-                recent_ctx_exists \u003d False\n-                local_ctx \u003d None\n-                # see if we have a context recent enough to be reported,\n-                # and grab the local replica\u0027s replication stats\n-                for c, ts in contexts:\n-                    ts_plus_timeout \u003d float(ts) + self.recon_sharded_timeout\n-                    if ts_plus_timeout \u003e\u003d float(Timestamp.now()):\n-                        recent_ctx_exists \u003d True\n-                    if (c.fresh_ref \u003d\u003d broker.get_info()[\u0027id\u0027]):\n-                        local_ctx \u003d c\n-\n-                if not recent_ctx_exists:\n-                    return\n-\n-                if local_ctx:\n-                    info[\"total_replicate_calls\"] \u003d local_ctx.replication_count\n-                    info[\"total_replicate_time\"] \u003d local_ctx.replication_time\n+            # grab the local context for replication stats, and while we\u0027re at\n+            # it check if there\u0027s any context updates (even from other nodes)\n+            # that are new enough to justify reporting this as \"in progress\"\n+            local_ctx \u003d None\n+            newest_update \u003d 0\n+            for c, ts in contexts:\n+                newest_update \u003d max(newest_update, float(ts))\n+                if (c.fresh_ref \u003d\u003d broker.get_info()[\u0027id\u0027]):\n+                    local_ctx \u003d c\n+\n+            # N.B. float(Timestamp.now()) is a surprising way to spell\n+            # time.time(), but patching the global time module\u0027s time function\n+            # can be problematic, and existing tests (e.g.\n+            # test_one_shard_cycle) already use mock_timestamp_now(ts_now)\n+            cutoff \u003d float(Timestamp.now()) - self.recon_sharded_timeout\n+            # we don\u0027t expect a sharded db to be missing context unless there\n+            # was an error so we\u0027ll report as in progress until a context\n+            # update tells us it\u0027s long done\n+            if newest_update and newest_update \u003c cutoff:\n+                return\n+\n+            if local_ctx:\n+                info[\"total_replicate_calls\"] \u003d local_ctx.replication_count\n+                info[\"total_replicate_time\"] \u003d local_ctx.replication_time\n \n         if processing_time:\n             info[\u0027processing_time\u0027] \u003d processing_time\n```\n\n^ which does de-dent, which is nice - and makes the return condition all on one line instead of:\n\n```\nif contexts:\n    ...\n    if not new_context:\n        ...\n```\n\nbut isn\u0027t immediately super obviously better?  I feel like there should be a more clever way to spell \"all_founc_ctx_are_old\" that doesn\u0027t return when there are no context to negate an initial hypothesis that \"all_founc_ctx_are_old \u003d False\"\n\nOr maybe if you sort the context first you can just bail early if the first one is ancient.","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":false,"context_lines":[{"line_number":1066,"context_line":"                        local_ctx \u003d c"},{"line_number":1067,"context_line":""},{"line_number":1068,"context_line":"                if not recent_ctx_exists:"},{"line_number":1069,"context_line":"                    return"},{"line_number":1070,"context_line":""},{"line_number":1071,"context_line":"                if local_ctx:"},{"line_number":1072,"context_line":"                    info[\"total_replicate_calls\"] \u003d local_ctx.replication_count"}],"source_content_type":"text/x-python","patch_set":3,"id":"846c79a4_5bb0567b","line":1069,"in_reply_to":"6f0d116f_1107dfe5","updated":"2025-11-07 20:25:29.000000000","message":"Done","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"21f39657b2e99ed432057ccdeee6ffd763e25809","unresolved":true,"context_lines":[{"line_number":1039,"context_line":"            # we check if the first one is new enough to warrant a report"},{"line_number":1040,"context_line":"            if contexts and ("},{"line_number":1041,"context_line":"                    float(contexts[0][1]) + self.recon_sharded_timeout \u003e\u003d"},{"line_number":1042,"context_line":"                    float(Timestamp.now())):"},{"line_number":1043,"context_line":"                recent_ctx_exists \u003d True"},{"line_number":1044,"context_line":""},{"line_number":1045,"context_line":"            if contexts and not recent_ctx_exists:"}],"source_content_type":"text/x-python","patch_set":18,"id":"a2bfd7de_8c882304","line":1042,"updated":"2025-12-05 18:31:08.000000000","message":"``contexts[0][1]`` is a string that usually will cast to a float but won\u0027t necessarily in general: Timestamps can have a complex string representation of the form ``\u003cfloat\u003e_\u003coffset\u003e``\n\nSo it\u0027s best to cast to a Timestamp and then compare Timestamp instances:\n\n``Timestamp(contexts[0][1]) \u003e\u003d Timestamp.now()``","commit_id":"5ceb8173983b1769484e5f4769cae73b5c7adbfc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"fad7c613d09497565dcdc0322f659125a8532511","unresolved":true,"context_lines":[{"line_number":1042,"context_line":"                    float(Timestamp.now())):"},{"line_number":1043,"context_line":"                recent_ctx_exists \u003d True"},{"line_number":1044,"context_line":""},{"line_number":1045,"context_line":"            if contexts and not recent_ctx_exists:"},{"line_number":1046,"context_line":"                return"},{"line_number":1047,"context_line":""},{"line_number":1048,"context_line":"            # get the local replica\u0027s replication stats"}],"source_content_type":"text/x-python","patch_set":18,"id":"603fcb6a_ed60d244","line":1045,"updated":"2025-12-13 22:18:10.000000000","message":"is it an intentional change that progress is now recorded when there are no contexts? before we returned ``if not contexts`` at line 1025","commit_id":"5ceb8173983b1769484e5f4769cae73b5c7adbfc"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"fad7c613d09497565dcdc0322f659125a8532511","unresolved":true,"context_lines":[{"line_number":1045,"context_line":"            if contexts and not recent_ctx_exists:"},{"line_number":1046,"context_line":"                return"},{"line_number":1047,"context_line":""},{"line_number":1048,"context_line":"            # get the local replica\u0027s replication stats"},{"line_number":1049,"context_line":"            for c, ts in contexts:"},{"line_number":1050,"context_line":"                if c.fresh_ref \u003d\u003d broker.get_info()[\u0027id\u0027]:"},{"line_number":1051,"context_line":"                    info[\"total_replicate_time\"] \u003d c.replication_time"}],"source_content_type":"text/x-python","patch_set":18,"id":"33522be4_99d7a064","line":1048,"updated":"2025-12-13 22:18:10.000000000","message":"I don\u0027t this change is covered by any test - in fact, I\u0027m not sure that the addition of these stats was ever covered by unit tests","commit_id":"5ceb8173983b1769484e5f4769cae73b5c7adbfc"}],"test/unit/container/test_sharder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b6a8ded07133ce6f7a9a0386068b7cbd5973c5bb","unresolved":true,"context_lines":[{"line_number":8363,"context_line":"                  \u0027misplaced_done\u0027: True}"},{"line_number":8364,"context_line":"        key \u003d \u0027X-Container-Sysmeta-Shard-Context-%s\u0027 % db_id"},{"line_number":8365,"context_line":"        broker.update_metadata("},{"line_number":8366,"context_line":"            {key: (json.dumps(params), Timestamp.now().internal)})"},{"line_number":8367,"context_line":""},{"line_number":8368,"context_line":"        # load successfully and populate fresh_ref"},{"line_number":8369,"context_line":"        ctx \u003d CleavingContext.load(broker)"}],"source_content_type":"text/x-python","patch_set":3,"id":"09f747c3_a73d0fdc","line":8366,"updated":"2025-11-05 22:43:05.000000000","message":"ok, so we\u0027re circumventing the cleaving_contxt.store() machinery to ensure we get some \"legacy\" cleaving context data.\n\nthis test does a good job of explaining the what - but not the WHY\n\nit is perhaps sufficient to have the comment in load:\n\n```\n# this is new so set it for backwards compat\n```\n\n... but better would be additional explanation in this test:\n\n```\ndiff --git a/test/unit/container/test_sharder.py b/test/unit/container/test_sharder.py\nindex fc31fca27..b2afab71c 100644\n--- a/test/unit/container/test_sharder.py\n+++ b/test/unit/container/test_sharder.py\n@@ -8347,33 +8347,46 @@ class TestCleavingContext(BaseTestSharder):\n         self.assertIn(db_id, str(cm.exception))\n         self.assertIn(mismatched_ref, str(cm.exception))\n \n-    def test_load_without_fresh_ref(self):\n+    def test_load_legacy_context_without_fresh_ref(self):\n         broker \u003d self._make_broker()\n         for i in range(6):\n             broker.put_object(\u0027o%s\u0027 % i, next(self.ts_iter).internal, 10,\n                               \u0027text/plain\u0027, \u0027etag_a\u0027, 0)\n+        broker.enable_sharding(Timestamp.now())\n+        self.assertTrue(broker.set_sharding_state())\n+        self.assertEqual(2, len(broker.get_brokers()))  # sanity!\n \n-        db_id \u003d broker.get_info()[\u0027id\u0027]\n-        params \u003d {\u0027ref\u0027: db_id,\n-                  \u0027cursor\u0027: \u0027curs\u0027,\n-                  \u0027max_row\u0027: 2,\n-                  \u0027cleave_to_row\u0027: 2,\n-                  \u0027last_cleave_to_row\u0027: 1,\n-                  \u0027cleaving_done\u0027: False,\n-                  \u0027misplaced_done\u0027: True}\n-        key \u003d \u0027X-Container-Sysmeta-Shard-Context-%s\u0027 % db_id\n-        broker.update_metadata(\n-            {key: (json.dumps(params), Timestamp.now().internal)})\n+        existing_ctx \u003d CleavingContext.load_all(broker)\n+        self.assertEqual(0, len(existing_ctx))\n \n-        # load successfully and populate fresh_ref\n+        new_ctx \u003d CleavingContext.load(broker)\n+        # simulate a legacy context saved before we added fresh_ref\n+        raw_ctx \u003d dict(new_ctx)\n+        raw_ctx.pop(\u0027fresh_ref\u0027)\n+        broker.set_sharding_sysmeta(\u0027Context-\u0027 + new_ctx.ref,\n+                                    json.dumps(raw_ctx))\n+        new_ctx.store(broker)\n+\n+        # we now have a legacy context saved in a old broker\n+        saved_ctx \u003d CleavingContext.load_all(broker)\n+        self.assertEqual(1, len(saved_ctx))\n+\n+        # we reload it with current swift\n+        loaded_ctx \u003d CleavingContext.load(broker)\n+        # and the fresh_ref backref is added to point to the current broker\n+        self.assertEqual(broker.get_info()[\u0027id\u0027], loaded_ctx.fresh_ref)\n+        # N.B. the ref is actually the retiring db ref\n+        self.assertNotEqual(loaded_ctx.ref, loaded_ctx.fresh_ref)\n+\n+    def test_load_manipulated_broken_fresh_ref(self):\n+        broker \u003d self._make_broker()\n+        broker.enable_sharding(Timestamp.now())\n         ctx \u003d CleavingContext.load(broker)\n-        self.assertEqual(db_id, ctx.ref)\n-        # fresh_ref and ref should be the same (not sharding so)\n-        # at the time of load brokers[0] should be \u003d\u003d brokers[-1]\n-        self.assertEqual(db_id, ctx.fresh_ref)\n-        self.assertEqual(\u0027curs\u0027, ctx.cursor)\n-        # test max row update\n-        self.assertEqual(6, ctx.max_row)\n+        self.assertEqual(ctx.ref, ctx.fresh_ref)  # does this make sense?\n+        ctx.fresh_ref \u003d None\n+        ctx.store(broker)\n+        ctx \u003d CleavingContext.load(broker)\n+        self.assertIsNone(ctx.fresh_ref)  # does this make sense?\n \n     def test_load_all(self):\n         broker \u003d self._make_broker()\n```","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":38368,"name":"Christian Ohanaja","display_name":"Christian Ohanaja","email":"cohanaja@nvidia.com","username":"cohanaja"},"change_message_id":"0d5850f19dd3a11f75f1c10d8cbe5fddb7ae950a","unresolved":false,"context_lines":[{"line_number":8363,"context_line":"                  \u0027misplaced_done\u0027: True}"},{"line_number":8364,"context_line":"        key \u003d \u0027X-Container-Sysmeta-Shard-Context-%s\u0027 % db_id"},{"line_number":8365,"context_line":"        broker.update_metadata("},{"line_number":8366,"context_line":"            {key: (json.dumps(params), Timestamp.now().internal)})"},{"line_number":8367,"context_line":""},{"line_number":8368,"context_line":"        # load successfully and populate fresh_ref"},{"line_number":8369,"context_line":"        ctx \u003d CleavingContext.load(broker)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3e51d1be_f444c3a3","line":8366,"in_reply_to":"09f747c3_a73d0fdc","updated":"2025-11-07 20:25:29.000000000","message":"Done","commit_id":"b539ea942615952d9031e9f3428d053e2a896663"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"fad7c613d09497565dcdc0322f659125a8532511","unresolved":true,"context_lines":[{"line_number":3447,"context_line":"        mocked.assert_called_once_with(\u0027sharding_in_progress\u0027, \u0027all\u0027, mock.ANY)"},{"line_number":3448,"context_line":""},{"line_number":3449,"context_line":"        # clear the contexts then run _record_sharding_progress"},{"line_number":3450,"context_line":"        # to make sure we still log stats for brokers without contexts"},{"line_number":3451,"context_line":"        for context, _ in CleavingContext.load_all(broker):"},{"line_number":3452,"context_line":"            context.delete(broker)"},{"line_number":3453,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"3d241f3b_a0a7655a","line":3450,"updated":"2025-12-13 22:18:10.000000000","message":"why was this changed i.e. to log when there are not contexts? doesn\u0027t it mean we keep logging sharded brokers that have had their old contexts cleaned up?","commit_id":"5ceb8173983b1769484e5f4769cae73b5c7adbfc"}]}