)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"d095ab18_cd16be82","updated":"2025-12-16 22:00:03.000000000","message":"\"fixing\" code that is using `float(ts_str)` when it could use `Timestamp(ts_str)` seems pretty safe - perhaps even benign (at least as long as the ts_str for the cleaving context doesn\u0027t have an offset)\n\nSending the shard update requests with `x-timestamp: now.internal` should be ok - we already had tests that asserted this, but since we don\u0027t expect Timestamp.now() to return an offset `ts.normal \u003d\u003d ts.internal`\n\nThe tests that prove the `float(Timestamp(ts_str)) + cutoff \u003c now` at least won\u0027t *blow up* seem a little on the nose b/c I don\u0027t really understand when/how we expect to get offsets into the context timestamps - maybe in a future patch?\n\nI think the plan is db metadata (or at least sharding metadata?) WILL start to get an default tx-offset when calling `Timestamp.now()`?\n\n```\n    def set_sharding_sysmeta(self, key, value):\n        \"\"\"\n        Updates the broker\u0027s metadata stored under the given key\n        prefixed with a sharding specific namespace.\n\n        :param key: metadata key in the sharding metadata namespace.\n        :param value: metadata value\n        \"\"\"\n        self.update_metadata({\u0027X-Container-Sysmeta-Shard-\u0027 + key:\n                              (value, Timestamp.now().internal)})\n```\n\n^ IIUC, I think in at least the two cases discovered here, we should still be fine.\n\n1) \"picking the most recently updated cleaving-context metadata\" \n\n^ this will probably change to use the fresh_ref in the future anyway: 966139: Improves recon stats cleaving context retrieval | https://review.opendev.org/c/openstack/swift/+/966139 - but if it doesn\u0027t arbitrarily letting one of the \"same timestamp float values\" when is simply good enough for the job and exactly how it already behaves.\n\n2) \"updating the metadata timestamp sent with the shard update request to use the internal/offset representation\" \n\n^ AFAICT we don\u0027t actually send any user/sys/save_header metadata along with the PUT shard request, but the PUT request validation requires it (mostly for the PUT_object request?) so either format is fine.","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0c3a5115cc588b5ef712a735ef17bb0653fafd10","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"42b636b9_08ab286d","updated":"2025-12-16 11:56:28.000000000","message":"Matt and I are both authors so waiting for another reviewer to merge","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"}],"swift/container/sharder.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0c3a5115cc588b5ef712a735ef17bb0653fafd10","unresolved":true,"context_lines":[{"line_number":1023,"context_line":"            contexts \u003d CleavingContext.load_all(broker)"},{"line_number":1024,"context_line":"            if not contexts:"},{"line_number":1025,"context_line":"                return"},{"line_number":1026,"context_line":"            context_ts \u003d max(float(ts) for c, ts in contexts)"},{"line_number":1027,"context_line":"            if context_ts + self.recon_sharded_timeout \\"},{"line_number":1028,"context_line":"                    \u003c float(Timestamp.now()):"},{"line_number":1029,"context_line":"                # last context timestamp too old for the"}],"source_content_type":"text/x-python","patch_set":5,"id":"f5d94ad4_d4985eb4","side":"PARENT","line":1026,"range":{"start_line":1026,"start_character":35,"end_line":1026,"end_character":37},"updated":"2025-12-16 11:56:28.000000000","message":"ts is a string, but it may not cast to a float if it has the form ``1234567890.12345_0000000000000001``","commit_id":"0eabd90388e88d5d45d8b3985f454c15798efcbb"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":true,"context_lines":[{"line_number":1023,"context_line":"            contexts \u003d CleavingContext.load_all(broker)"},{"line_number":1024,"context_line":"            if not contexts:"},{"line_number":1025,"context_line":"                return"},{"line_number":1026,"context_line":"            context_ts \u003d max(float(ts) for c, ts in contexts)"},{"line_number":1027,"context_line":"            if context_ts + self.recon_sharded_timeout \\"},{"line_number":1028,"context_line":"                    \u003c float(Timestamp.now()):"},{"line_number":1029,"context_line":"                # last context timestamp too old for the"}],"source_content_type":"text/x-python","patch_set":5,"id":"510920b1_a32ce5bf","side":"PARENT","line":1026,"range":{"start_line":1026,"start_character":35,"end_line":1026,"end_character":37},"in_reply_to":"f5d94ad4_d4985eb4","updated":"2025-12-16 22:00:03.000000000","message":"only two tests use the \"timestamp w/ offset\" pattern:\n\n```\nFAILED swift/test/unit/container/test_sharder.py::TestSharder::test_send_shard_ranges - AssertionError: \u00271765919212.45818_0000000000000001\u0027 !\u003d \u00271765919212.45818\u0027\nFAILED swift/test/unit/container/test_sharder.py::TestSharder::test_sharded_record_sharding_progress_tolerates_timestamp_offset - ValueError: could not convert string to float: \u00271765919240.00000_000000000000000d\u0027\n```\n\n... and they\u0027re both sort of forcing the offset just to prove this code is correct with `s/float/Timestamp/` - they don\u0027t do anything to justify why it might be reasonable for these state transitions to have an offset or why we can ignore them when picking \"the most recent\"\n\n```\n(Pdb) contexts\n[(CleavingContext(ref\u003d\u0027f2dbdf12-f65e-41a8-b1b1-3f39ff70f753-sda\u0027, cursor\u003d\u0027\u0027, max_row\u003d1, cleave_to_row\u003d2, last_cleave_to_row\u003dNone, cleaving_done\u003dTrue, misplaced_done\u003dTrue, ranges_done\u003d0, ranges_todo\u003d0, replication_time\u003d0), \u00271765919462.00000_000000000000000d\u0027), (CleavingContext(ref\u003d\u0027bcf930f4-91eb-477c-a5d8-c1d4cf49f96c-sda\u0027, cursor\u003d\u0027\u0027, max_row\u003d2, cleave_to_row\u003d2, last_cleave_to_row\u003dNone, cleaving_done\u003dTrue, misplaced_done\u003dTrue, ranges_done\u003d0, ranges_todo\u003d0, replication_time\u003d0), \u00271765919466.00000_000000000000000f\u0027)]\n(Pdb) contexts[0]\n(CleavingContext(ref\u003d\u0027f2dbdf12-f65e-41a8-b1b1-3f39ff70f753-sda\u0027, cursor\u003d\u0027\u0027, max_row\u003d1, cleave_to_row\u003d2, last_cleave_to_row\u003dNone, cleaving_done\u003dTrue, misplaced_done\u003dTrue, ranges_done\u003d0, ranges_todo\u003d0, replication_time\u003d0), \u00271765919462.00000_000000000000000d\u0027)\n(Pdb) contexts[0][1]\n\u00271765919462.00000_000000000000000d\u0027\n(Pdb) float(Timestamp(contexts[0][1]))\n1765919462.0\n```","commit_id":"0eabd90388e88d5d45d8b3985f454c15798efcbb"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":true,"context_lines":[{"line_number":1030,"context_line":"                # broker to be recorded"},{"line_number":1031,"context_line":"                return"},{"line_number":1032,"context_line":""},{"line_number":1033,"context_line":"            contexts_sorted \u003d sorted(contexts, key\u003dlambda x: float(x[1]))"},{"line_number":1034,"context_line":"            sharded_ctx \u003d contexts_sorted[-1]"},{"line_number":1035,"context_line":""},{"line_number":1036,"context_line":"        update_own_shard_range_stats(broker, own_shard_range)"}],"source_content_type":"text/x-python","patch_set":5,"id":"67b473de_6d372227","side":"PARENT","line":1033,"updated":"2025-12-16 22:00:03.000000000","message":"oic, the existing code was sort of in-efficient - we\u0027ve just lifted the sort earlier so we can avoid double calculating the max.","commit_id":"0eabd90388e88d5d45d8b3985f454c15798efcbb"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"0c3a5115cc588b5ef712a735ef17bb0653fafd10","unresolved":true,"context_lines":[{"line_number":1214,"context_line":"        headers.update({\u0027X-Backend-Record-Type\u0027: RECORD_TYPE_SHARD,"},{"line_number":1215,"context_line":"                        USE_REPLICATION_NETWORK_HEADER: \u0027True\u0027,"},{"line_number":1216,"context_line":"                        \u0027User-Agent\u0027: \u0027container-sharder %s\u0027 % os.getpid(),"},{"line_number":1217,"context_line":"                        \u0027X-Timestamp\u0027: Timestamp.now().internal,"},{"line_number":1218,"context_line":"                        \u0027Content-Length\u0027: len(body),"},{"line_number":1219,"context_line":"                        \u0027Content-Type\u0027: \u0027application/json\u0027})"},{"line_number":1220,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"10eb0071_89714e66","line":1217,"updated":"2025-12-16 11:56:28.000000000","message":"+1, x-timestamp in backend *requests* should always have full precision i.e. internal format","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":true,"context_lines":[{"line_number":1214,"context_line":"        headers.update({\u0027X-Backend-Record-Type\u0027: RECORD_TYPE_SHARD,"},{"line_number":1215,"context_line":"                        USE_REPLICATION_NETWORK_HEADER: \u0027True\u0027,"},{"line_number":1216,"context_line":"                        \u0027User-Agent\u0027: \u0027container-sharder %s\u0027 % os.getpid(),"},{"line_number":1217,"context_line":"                        \u0027X-Timestamp\u0027: Timestamp.now().internal,"},{"line_number":1218,"context_line":"                        \u0027Content-Length\u0027: len(body),"},{"line_number":1219,"context_line":"                        \u0027Content-Type\u0027: \u0027application/json\u0027})"},{"line_number":1220,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"8a3a7f00_f0888c30","line":1217,"in_reply_to":"10eb0071_89714e66","updated":"2025-12-16 22:00:03.000000000","message":"this change is covered by: `swift/test/unit/container/test_sharder.py::TestSharder::test_send_shard_ranges`\n\nactually it\u0027s maybe a little curious why we would make up this timestamp to just be whenever the sharder happened to get to this replica... I think the container-server is using this timestamp to update user/sys metadata:\n\n```\n    @timing_stats()\n    def PUT_shard(self, req, broker, account, req_timestamp):\n        \"\"\"Put shards into container.\"\"\"\n        requested_policy_index \u003d self.get_and_validate_policy_index(req)\n        try:\n            # validate incoming data...\n            shard_ranges \u003d [ShardRange.from_dict(sr)\n                            for sr in json.loads(req.body)]\n        except (ValueError, KeyError, TypeError) as err:\n            return HTTPBadRequest(\u0027Invalid body: %r\u0027 % err)\n        created \u003d self._maybe_autocreate(\n            broker, req_timestamp, account, requested_policy_index, req)\n        self._update_metadata(req, broker, req_timestamp, \u0027PUT\u0027)\n        if shard_ranges:\n            # TODO: consider writing the shard ranges into the pending\n            # file, but if so ensure an all-or-none semantic for the write\n            broker.merge_shard_ranges(shard_ranges)\n        return self._create_ok_resp(req, broker, created)\n```\n\n... of which there is none on this request?\n\nis this `Timestamp.now()` going to start auto inserting a tx-offset automatically in a future patch?  Even if so I expect it won\u0027t have any effect on the result.","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"}],"test/unit/container/test_sharder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":true,"context_lines":[{"line_number":2279,"context_line":"                db_hash \u003d hash_path(acceptor.account,"},{"line_number":2280,"context_line":"                                    acceptor.container)"},{"line_number":2281,"context_line":"                # NB expected cleaved db name includes acceptor epoch"},{"line_number":2282,"context_line":"                db_name \u003d \u0027%s_%s.db\u0027 % (db_hash, acceptor_epoch.normal)"},{"line_number":2283,"context_line":"                expected_acceptor_dbs.append("},{"line_number":2284,"context_line":"                    os.path.join(self.tempdir, \u0027sda\u0027, \u0027containers\u0027, \u00270\u0027,"},{"line_number":2285,"context_line":"                                 db_hash[-3:], db_hash, db_name))"}],"source_content_type":"text/x-python","patch_set":5,"id":"5e96aed9_6093a925","line":2282,"updated":"2025-12-16 22:00:03.000000000","message":"the use of \"normal\" seems to best match the real:\n\n```\ndef make_db_file_path(db_path, epoch):\n    \"\"\"\n    Given a path to a db file, return a modified path whose filename part has\n    the given epoch.\n\n    A db filename takes the form ``\u003chash\u003e[_\u003cepoch\u003e].db``; this method replaces\n    the ``\u003cepoch\u003e`` part of the given ``db_path`` with the given ``epoch``\n    value, or drops the epoch part if the given ``epoch`` is ``None``.\n\n    :param db_path: Path to a db file that does not necessarily exist.\n    :param epoch: A string (or ``None``) that will be used as the epoch\n        in the new path\u0027s filename; non-``None`` values will be\n        normalized to the normal string representation of a\n        :class:`~swift.common.utils.Timestamp`.\n    :return: A modified path to a db file.\n    :raises ValueError: if the ``epoch`` is not valid for constructing a\n        :class:`~swift.common.utils.Timestamp`.\n    \"\"\"\n    hash_, _, ext \u003d parse_db_filename(db_path)\n    db_dir \u003d os.path.dirname(db_path)\n    if epoch is None:\n        return os.path.join(db_dir, hash_ + ext)\n    epoch \u003d Timestamp(epoch).normal\n    return os.path.join(db_dir, \u0027%s_%s%s\u0027 % (hash_, epoch, ext))\n```","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":true,"context_lines":[{"line_number":5399,"context_line":"                        # we don\u0027t expect these PUTs to have offsets but it\u0027s"},{"line_number":5400,"context_line":"                        # used here to verify that the internal format of the"},{"line_number":5401,"context_line":"                        # Timestamp is used for X-Timestamp"},{"line_number":5402,"context_line":"                        now.offset \u003d 1"},{"line_number":5403,"context_line":"                        res \u003d sharder._send_shard_ranges("},{"line_number":5404,"context_line":"                            broker, \u0027a\u0027, \u0027c\u0027, shard_ranges)"},{"line_number":5405,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"7b59c80f_e271461e","line":5402,"updated":"2025-12-16 22:00:03.000000000","message":"I wonder if \n\n```\n# we don\u0027t expect this\nnow.offset \u003d 1\n```\n\n... would look better as:\n\n```\nself.assertEqual(now.offset, 0, \"we do not expect this\")\nnow.offset \u003d 1\n```","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f5d38234272b019b4987889e1cb8dc3ab36be0a6","unresolved":true,"context_lines":[{"line_number":5421,"context_line":"                self.assertEqual(\u00270\u0027, path_parts[1])"},{"line_number":5422,"context_line":"                self.assertEqual(\u0027PUT\u0027, req[\u0027method\u0027])"},{"line_number":5423,"context_line":"                self.assertEqual([\u0027a\u0027, \u0027c\u0027], path_parts[-2:])"},{"line_number":5424,"context_line":"                req_headers \u003d req[\u0027headers\u0027]"},{"line_number":5425,"context_line":"                for k, v in expected_headers.items():"},{"line_number":5426,"context_line":"                    self.assertEqual(v, req_headers[k])"},{"line_number":5427,"context_line":"                self.assertTrue("}],"source_content_type":"text/x-python","patch_set":5,"id":"230f6db9_ec0ea07c","line":5424,"updated":"2025-12-16 22:00:03.000000000","message":"fwiw if you look closely at these headers you can see that the offset is in fact 1\n\n```\n\u003e /home/vagrant/swift/test/unit/container/test_sharder.py(5426)do_test()\n-\u003e req_headers \u003d req[\u0027headers\u0027]\n(Pdb) n\n\u003e /home/vagrant/swift/test/unit/container/test_sharder.py(5427)do_test()\n-\u003e for k, v in expected_headers.items():\n(Pdb) !req_headers\n{\u0027X-Backend-Record-Type\u0027: \u0027shard\u0027, \u0027X-Backend-Use-Replication-Network\u0027: \u0027True\u0027, \u0027User-Agent\u0027: \u0027container-sharder 8581\u0027, \u0027X-Timestamp\u0027: \u00271765920535.37898_0000000000000001\u0027, \u0027Content-Length\u0027: \u0027553\u0027, \u0027Content-Type\u0027: \u0027application/json\u0027, \u0027X-Backend-Allow-Reserved-Names\u0027: \u0027true\u0027}\n```","commit_id":"9abd8ae71ec385e7687a11b50d7473be65eb9341"}]}