)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":6968,"name":"Christian Schwede","email":"cschwede@nvidia.com","username":"cschwede"},"change_message_id":"bc9e0e8586ab7101db54ff651b2b96ef21bd2159","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"1befa567_b5afbe4f","updated":"2026-06-27 09:24:45.000000000","message":"recheck\n\nRun timed out shortly before finishing uploading logs. Unrelated to this change","commit_id":"dcdd97de28ab153eb5f436386e96c0c138f5fa22"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"31362792e87cc1f37e895292777137087be818b9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"e6352411_36c56b6d","updated":"2026-09-01 23:31:59.000000000","message":"\u003e I like the direction here: carry enough placement history to make old-primary recovery and repair scheduling more deliberate. However, I think the builder’s state model needs one more round of design before this foundation is safe to merge.\n  \u003e\n  \u003e Blocking concern — 991132, RingBuilder._part_has_live_history()\n  \u003e\n  \u003e The move gate treats history[r][p] \u003d\u003d current[r][p] as proof that no relevant last_moved[r][p] context remains. That conflates two different facts:\n  \u003e\n  \u003e - whether there is a useful prior source to publish to ring consumers; and\n  \u003e - how recently the current replica assignment was made.\n  \u003e\n  \u003e In particular, when the device named in history fails after\n  \u003e failure_grace_cycles but before max_history_cycles, the failure sweep\n  \u003e makes history inert while leaving its age young. The whole-part move gate\n  \u003e then opens as soon as legacy min_part_hours permits it, even though that\n  \u003e current assignment is still within the intended stability window.\n  \u003e\n  \u003e Please retain an independent per-row placement-age meaning even when\n  \u003e history \u003d\u003d current. The partition-wide invariant can remain unchanged:\n  \u003e any young replica row should still prevent another voluntary move of the\n  \u003e partition.\n  \u003e\n\nThis is the inline comment I left at the history !\u003d current predicate.\n\n  \u003e\n  \u003e State migration / duplicate bookkeeping\n  \u003e\n  \u003e Once placement age has that independent meaning, it is worth reconsidering\n  \u003e whether _last_part_moves must remain a permanent parallel per-partition\n  \u003e structure. An old builder’s per-part age can be conservatively migrated into\n  \u003e every replica row: rows for a part that has not yet met min_part_hours\n  \u003e become fresh/young; already-eligible parts become expired. Exact remaining\n  \u003e hour precision is lost, but conservatively extending a remaining cooldown is\n  \u003e safer than retaining two authoritative movement clocks indefinitely.\n  \u003e\n\nI have implicitly decided that \"rollback\" from a builder that\u0027s been migrated can just pretend-min-part-hours-passed for all I care; or they can use backups - don\u0027t care.  I do NOT want to maintain both FOREVER just so that we can have smooth \"new code still works\" - the old min-part-hours will never be as great as history_r2p2d long live lastmoved_r2p2d!!!\n\n  \u003e Failure history needs a bounded urgency window\n  \u003e\n  \u003e NONE_DEV is not proof that repair remains incomplete; it records that a\n  \u003e replica was lost and that no usable prior primary remains. The ring cannot\n  \u003e observe when replication/reconstruction has caught up, so retaining this\n  \u003e marker for the full max_history_cycles window repeatedly gives old failure\n  \u003e work top billing whenever a new ring restarts the daemon scan.\n  \u003e\n  \u003e We want a newly failed device to outrank ordinary work, not every failure\n  \u003e from the past several days to remain equally urgent. If everything is urgent,\n  \u003e nothing is urgent.\n  \u003e\n  \u003e Please add an explicit short NONE_DEV TTL—e.g.\n  \u003e failure_history_cycles—after which the history hint becomes inert. Keep\n  \u003e that separate from the placement-age move hold: expiry should demote stale\n  \u003e repair work without authorizing another voluntary move of the replacement\n  \u003e replica.\n\n  \u003e\n  \u003e Operator/compatibility contract\n  \u003e\n  \u003e The builder has retention knobs but no corresponding swift-ring-builder\n  \u003e operator commands or history inspection surface.\n  \nPlease consider if we could try and cook up something here?  idk - maybe like \"dispersion\" but for ... a count/age based 0.0-100% gradient on \"how much activity the part has had\" where more replicas failed recently is higher and ring closer to 0 is relatively stable?","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bc3d84fef5f1edce751f2d4eb8c05ca266d127b5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"6458a751_02e9d256","updated":"2026-08-28 21:13:43.000000000","message":"I\u0027m still working my way into the actual ring and builder code - this one comment is just cause the cli changes are spread across two patches\n\nI think it would be better if this moved, or the series collapsed into one large (~3K line) but *complete* change","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":38496,"name":"Andressa Cabistani","display_name":"Andressa","email":"acabistani@gmail.com","username":"andressadotpy","status":"I\u0027m a Software Engineer at Red Hat and I love Open Source and connect with people! Feel free to DM through IRC, I\u0027ll be delighted to chat"},"change_message_id":"74647f3b5f1a322df7268ed3c8e0cf28afd363fe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"5eeafa91_135dea08","updated":"2026-07-28 14:36:35.000000000","message":"Looks good to me. Here a few things I analyzed:\n\n- Great test coverage\n- Tested live-history gate: age\u003d0 blocks, age\u003dthreshold releases (strict \u003c)\n- Tested ring v2 persistence: history round-trips, tier classification works (1 important, 2 normal), v1 excludes history\n- Tested pretend_min_part_hours_passed: ages set to 0xff, gate released\n- And overall code quality","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"fe34d1f998d1cad2a87200a29b4cdd83e95d707f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"0cac87e1_86cefdd8","updated":"2026-07-31 04:49:36.000000000","message":"Yeah, this is looking good, some comments in-line but nothing I can see as blocking.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3778d7a5e92b0dac941ed02fd3726031da6b6233","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"47dd0c74_0a251db5","updated":"2026-09-01 23:34:00.000000000","message":"forgot about this one:\n\n  \u003e Granularity of the move gate\n  \u003e\n  \u003e This stack implements only a partition-wide embargo:\n  \u003e RingBuilder._can_part_move(part) has no replica argument, and every gather\n  \u003e path asks the same whole-part question. Thus one live/young row prevents\n  \u003e every other replica row of that partition from moving.\n  \u003e\n  \u003e Please make that an explicit policy decision. It may be the right way to\n  \u003e preserve the existing “only one replica of a partition in flight” invariant,\n  \u003e but it is not the only interpretation of per-replica history. If the desired\n  \u003e rule is instead “do not move this recently assigned replica again,” the\n  \u003e builder needs a can_part_move(part, replica)-style predicate and corresponding\n  \u003e gather-path changes.\n  \u003e\n  \u003e Importantly, retaining independent per-row placement age does not require\n  \u003e relaxing the current partition-wide invariant. It merely makes that choice\n  \u003e explicit: a partition-wide gate can aggregate it as “any row is still young.”","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"30550f31_f8143905","updated":"2026-08-31 22:36:39.000000000","message":"this was my first in-roads to the ring itself - what are the new interfaces; how are they consumed.\n\nLuckily it\u0027s a pretty straight forward 1:1 mapping with \n\nget_part_old_nodes \u003d\u003e proxy change\nget_part_[severity] \u003d\u003e reconstructor change\n\n... unfortunatly I found myself unconvinced on either shape and it left me questioning if either implementation actually uses the history table the way we want.  I\u0027m confidently -1 because I don\u0027t want to overload the term \"tiers\" in the ring interface for history \"severity\"\n\n... but also maybe that\u0027s not the right interface; and I think mostly both of these new interfaces belong with the code that uses them since they are hard to evaluate standalone w/o context.\n\nI\u0027ll look at the builder changes tomorrow.  But I think the patch chain I\u0027d like to see is much shorter:\n\n1) add all the builder stuff I haven\u0027t looked at yet\n2) add ring stuff fold \"get_more_old_nodes\" and \"fix\" `[404, 507, Timeout]`\n3) add ring stuff for \"skip_moved_sync_jobs\" and \"fix\" `handoffs_only`","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"}],"swift/cli/ringbuilder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"bc3d84fef5f1edce751f2d4eb8c05ca266d127b5","unresolved":true,"context_lines":[{"line_number":629,"context_line":"                # builder\u0027s would falsely report obsolete after a rebalance."},{"line_number":630,"context_line":"                if (loaded_ring.format_version or 1) \u003c 2:"},{"line_number":631,"context_line":"                    builder_dict.pop(\u0027history\u0027, None)"},{"line_number":632,"context_line":"                    ring_dict.pop(\u0027history\u0027, None)"},{"line_number":633,"context_line":"                if builder_dict \u003d\u003d ring_dict:"},{"line_number":634,"context_line":"                    print(\u0027Ring file %s is up-to-date\u0027 % ring_file)"},{"line_number":635,"context_line":"                else:"}],"source_content_type":"text/x-python","patch_set":3,"id":"111598f6_c3fff5ea","line":632,"updated":"2026-08-28 21:13:43.000000000","message":"what is the point of this - we\u0027re poping a value that shouldn\u0027t exist because it\u0027s an old version\n\nthis is the \"default\" command - it\u0027s just printing out stuff about the ring - I don\u0027t think it prints any history stuff even if it *is* v2\n\nso why do we care about this?","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"}],"swift/common/ring/builder.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"31362792e87cc1f37e895292777137087be818b9","unresolved":true,"context_lines":[{"line_number":236,"context_line":"        for r, hist_row in enumerate(history):"},{"line_number":237,"context_line":"            if part \u003e\u003d len(hist_row):"},{"line_number":238,"context_line":"                continue"},{"line_number":239,"context_line":"            if (hist_row[part] !\u003d current[r][part]"},{"line_number":240,"context_line":"                    and ages[r][part] \u003c threshold):"},{"line_number":241,"context_line":"                return True"},{"line_number":242,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":3,"id":"16dbf6f6_a32595ef","line":239,"updated":"2026-09-01 23:31:59.000000000","message":"\u003e Why is history !\u003d current part of the move gate? It makes\n  \u003e last_moved[r][part] irrelevant as soon as we intentionally discard an\n  \u003e old-primary hint.\n  \u003e\n  \u003e For example, if the device in history[r][part] later fails after\n  \u003e failure_grace_cycles but before max_history_cycles, the failure sweep\n  \u003e sets history[r][part] \u003d current[r][part] while leaving the age young.\n  \u003e This predicate then releases the history gate as soon as the legacy\n  \u003e min_part_hours gate permits it, even though the current replica\n  \u003e assignment is still within its intended stability window.\n  \u003e\n  \u003e “Where did this replica come from?” and “how recently was this replica\n  \u003e assigned?” seem like separate facts. Could we retain an independent\n  \u003e per-row placement-age gate when history \u003d\u003d current? Besides avoiding\n  \u003e that churn, it would let us expire NONE_DEV as a short-lived urgent\n  \u003e repair signal without also declaring the replacement primary safe to\n  \u003e move again.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":247,"context_line":"        # but _has_part_moved will."},{"line_number":248,"context_line":"        return (self._last_part_moves[part] \u003e\u003d self.min_part_hours and"},{"line_number":249,"context_line":"                not self._has_part_moved(part) and"},{"line_number":250,"context_line":"                not self._part_has_live_history(part))"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"    @contextmanager"},{"line_number":253,"context_line":"    def debug(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"e0a95fea_1150335e","line":250,"updated":"2026-08-31 22:36:39.000000000","message":"When I look at the two previous diffs they are both builder only\n\nSo you might think a logical seperation would be:\n\n1) introduce the history \u0026 r2p2last_moved in the builder so that we can give rings a history_r2p2d\n2) expose some public interfaces over the history_r2p2d\n\n... but even tho this is the first change to introduce any changes to the ring interface itself - it ALSO *still* working on making the builder/rebalance know how to use  `_replica2part2last_moved`\n\nmeaning that the builder changes are spread over (at least?) *3* changes","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"fe34d1f998d1cad2a87200a29b4cdd83e95d707f","unresolved":true,"context_lines":[{"line_number":1228,"context_line":"        if self._replica2part2last_moved is not None:"},{"line_number":1229,"context_line":"            for age_row in self._replica2part2last_moved:"},{"line_number":1230,"context_line":"                for p in range(len(age_row)):"},{"line_number":1231,"context_line":"                    age_row[p] \u003d 0xff"},{"line_number":1232,"context_line":""},{"line_number":1233,"context_line":"    def get_part_devices(self, part):"},{"line_number":1234,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"9562af7e_06964d73","line":1231,"updated":"2026-07-31 04:49:36.000000000","message":"OK this is where this new design is a little problematic. We\u0027ll have min_part_hours which should pass but then we have max_history_cycles which means this live in history which is a multiplier for min_part_hours. So what should happen in this case?\n\nIt seems to clear the history too, is that what it should do? The function name pretend_min_part_hours_passed would indicate only one histroy cycle is complete... but we know this function really just clears the blocks so we can do a proper rebalance again. I mean, even the doc string basically says, jump to 255 so what you\u0027re doing here is correct, but I really don\u0027t like this name any more.. but for legacy reasons it needs to stay.. but still it feels confusing.\n\nNot that I know what to do here, but annoys me 😉","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"}],"swift/common/ring/ring.py":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"fe34d1f998d1cad2a87200a29b4cdd83e95d707f","unresolved":true,"context_lines":[{"line_number":103,"context_line":"            raise ValueError("},{"line_number":104,"context_line":"                \u0027history row count %d does not match assignment %d\u0027"},{"line_number":105,"context_line":"                % (len(history), len(current)))"},{"line_number":106,"context_line":"        for r, hist_row in enumerate(history):"},{"line_number":107,"context_line":"            cur_row \u003d current[r]"},{"line_number":108,"context_line":"            if len(hist_row) !\u003d len(cur_row):"},{"line_number":109,"context_line":"                raise ValueError("}],"source_content_type":"text/x-python","patch_set":3,"id":"c2a114a4_a9ca5d72","line":106,"updated":"2026-07-31 04:49:36.000000000","message":"Could also:\n```\nfor r (hist_row, cur_row) in enumerate(zip(history, current)):\n```\n\nBut maybe your way is more readable. So cool.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"fe34d1f998d1cad2a87200a29b4cdd83e95d707f","unresolved":true,"context_lines":[{"line_number":222,"context_line":"        if \u0027swift/ring/history\u0027 in reader:"},{"line_number":223,"context_line":"            with reader.open_section(\u0027swift/ring/history\u0027) as section:"},{"line_number":224,"context_line":"                ring_dict[\u0027history\u0027] \u003d section.read_ring_table("},{"line_number":225,"context_line":"                    ring_dict[\u0027dev_id_bytes\u0027], partition_count)"},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"        return ring_dict"},{"line_number":228,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"aa72cfa0_2e958eb9","line":225,"updated":"2026-07-31 04:49:36.000000000","message":"Nice, it\u0027s cool with this history structure we can just use the existing read_ring_table as it has the same shape, dev_id_bytes and historic index is also saved, love it.\n\nThe new moved structure only exists in the builder pickle so not required here too. (for those playing along at home).\n\nOne day, I do want to merge the 2 (builders and rings v2 that is)","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":395,"context_line":"            self._dev_id_bytes \u003d ring_data._dev_id_bytes"},{"line_number":396,"context_line":"            self._replica2part2dev_id \u003d ring_data._replica2part2dev_id"},{"line_number":397,"context_line":"            self._history_replica2part2dev_id \u003d getattr("},{"line_number":398,"context_line":"                ring_data, \u0027_history_replica2part2dev_id\u0027, None)"},{"line_number":399,"context_line":"            self._part_shift \u003d ring_data._part_shift"},{"line_number":400,"context_line":"            self._rebuild_tier_data()"},{"line_number":401,"context_line":"            self._update_bookkeeping()"}],"source_content_type":"text/x-python","patch_set":3,"id":"fd505c41_214873be","line":398,"updated":"2026-08-31 22:36:39.000000000","message":"there is NEVER a case where `self._history_replica2part2dev_id` doesn\u0027t exist - if the `ring_data` from disk doesn\u0027t have it (old, v1, w/e) then this gets set explicitly to `None`\n\n... which is IMHO correct and sufficient that we can stop checking for `gettar(self, \u0027_history_replica2part2dev_id\u0027)`\n\nThe REAL question is if `None` is a better deffault than `_replica2part2dev_id` so that old rings can compare unconditionally - but the answer for old rings is just \"always current\"","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":536,"context_line":"                if dev_id not in seen_ids:"},{"line_number":537,"context_line":"                    part_nodes.append(self.devs[dev_id])"},{"line_number":538,"context_line":"                    seen_ids.add(dev_id)"},{"line_number":539,"context_line":"        return [dict(node, index\u003di) for i, node in enumerate(part_nodes)]"},{"line_number":540,"context_line":""},{"line_number":541,"context_line":"    def get_part_tiers(self, part):"},{"line_number":542,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"a42be361_6916ea33","line":539,"updated":"2026-08-31 22:36:39.000000000","message":"to compare: this is what a \"normal\" replica loop looks like - and I think `seen_ids` is vistigial.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":542,"context_line":"        \"\"\""},{"line_number":543,"context_line":"        Classify each replica row holding ``part`` against the history table."},{"line_number":544,"context_line":""},{"line_number":545,"context_line":"        :returns: ``(replica_index, tier)`` pairs by replica index, where tier"},{"line_number":546,"context_line":"                  is ``\u0027normal\u0027`` (no/equal history), ``\u0027urgent\u0027`` (history is"},{"line_number":547,"context_line":"                  the ``NONE_DEV`` sentinel), or ``\u0027important\u0027`` (a real,"},{"line_number":548,"context_line":"                  different device)."}],"source_content_type":"text/x-python","patch_set":3,"id":"75ea5406_6816d72a","line":545,"updated":"2026-08-31 22:36:39.000000000","message":"where tier is ... \"this totally new different thing from ring builer\u0027s tiers\" (!!!)\n\n```\n  Existing builder tiers mean placement hierarchy, roughly:\n\n  (region)\n  (region, zone)\n  (region, zone, ip)\n  (region, zone, ip, device)\n```\n\n*IF* we need a way to get a list of `(replica_index, ENUM)` I would beg we plase call it *anything* OTHER than \"tiers\"","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":550,"context_line":"        if time() \u003e self._rtime:"},{"line_number":551,"context_line":"            self._reload()"},{"line_number":552,"context_line":"        history \u003d getattr(self, \u0027_history_replica2part2dev_id\u0027, None)"},{"line_number":553,"context_line":"        none_dev \u003d none_dev_id(self._dev_id_bytes)"},{"line_number":554,"context_line":"        tiers \u003d []"},{"line_number":555,"context_line":"        for r, r2p2d in enumerate(self._replica2part2dev_id):"},{"line_number":556,"context_line":"            if part \u003e\u003d len(r2p2d):"}],"source_content_type":"text/x-python","patch_set":3,"id":"d4c9ff8e_5d5d4f1f","line":553,"updated":"2026-08-31 22:36:39.000000000","message":"we\u0027re already assuming both tables have the same device width!","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":552,"context_line":"        history \u003d getattr(self, \u0027_history_replica2part2dev_id\u0027, None)"},{"line_number":553,"context_line":"        none_dev \u003d none_dev_id(self._dev_id_bytes)"},{"line_number":554,"context_line":"        tiers \u003d []"},{"line_number":555,"context_line":"        for r, r2p2d in enumerate(self._replica2part2dev_id):"},{"line_number":556,"context_line":"            if part \u003e\u003d len(r2p2d):"},{"line_number":557,"context_line":"                continue"},{"line_number":558,"context_line":"            if history is None or r \u003e\u003d len(history):"}],"source_content_type":"text/x-python","patch_set":3,"id":"6a93e061_fcd83f9b","line":555,"updated":"2026-08-31 22:36:39.000000000","message":"This reads like we\u0027re about to iterate the whole `_replica2part2dev_id` - which is a smell for a method that accepts a part; but really we\u0027re just evaluating every replica:\n\n```\nfor r in replicas:\n    if placement[replica][part] !\u003d history[replica][part]:\n        recently_moved +\u003d 1\n```","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":554,"context_line":"        tiers \u003d []"},{"line_number":555,"context_line":"        for r, r2p2d in enumerate(self._replica2part2dev_id):"},{"line_number":556,"context_line":"            if part \u003e\u003d len(r2p2d):"},{"line_number":557,"context_line":"                continue"},{"line_number":558,"context_line":"            if history is None or r \u003e\u003d len(history):"},{"line_number":559,"context_line":"                tiers.append((r, \u0027normal\u0027))"},{"line_number":560,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"41e549ab_7b9d99fe","line":557,"updated":"2026-08-31 22:36:39.000000000","message":"I think this is for fractional replica count - and it probably has to be maintained - you can have some parts with 3 replicas and some with only 2\n\n```\n[\n  [1, 2, 3, 4],\n  [5, 6, 7, 8],\n  [9, 0],\n]\n```\n\nif you resize the main `r2p2d` such that you get `2.75` replicas IMHO BOTH tables should get resized; but when you as for `get_part_nodes(3)` it\u0027s still going to return only 2 replicas, and so there should be only 2 histories.\n\nI\u0027m not sure what to do about history that get\u0027s *smaller* - I don\u0027t know how often people reduce the number of replicas - it seems like \"the 3rd replica of this 2-replica part WAS only devN\" would be useful information to keep; but I don\u0027t like the implication of keeping a whole 4rd replica of history during a 4\u003d\u003e3 replica reduction - it seems like that history should just be assigned to one of the other un-moved replicas?  And none of this applies to EC - it can\u0027t do fraction replicas or replica count changes.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":555,"context_line":"        for r, r2p2d in enumerate(self._replica2part2dev_id):"},{"line_number":556,"context_line":"            if part \u003e\u003d len(r2p2d):"},{"line_number":557,"context_line":"                continue"},{"line_number":558,"context_line":"            if history is None or r \u003e\u003d len(history):"},{"line_number":559,"context_line":"                tiers.append((r, \u0027normal\u0027))"},{"line_number":560,"context_line":"                continue"},{"line_number":561,"context_line":"            hist_row \u003d history[r]"}],"source_content_type":"text/x-python","patch_set":3,"id":"3020c30d_885aea22","line":558,"updated":"2026-08-31 22:36:39.000000000","message":"`r \u003e\u003d len(history)` is suggesting to me that we think we can have a `_history_replica2part2dev_id` that isn\u0027t the same size/shape as `_replica2part2dev_id` ... and if that\u0027s true today I should very much like to ensure that we resize the history table when we right ring such that it is *always* the same size and shape as the actual `_replica2part2dev_id` table\n\nIf the `_replica2part2dev_id` has changed size during a recent rebalance (e.g. replica count or part power increase) the \"history\" for those new replicas/parts would just be equal to the current assignment - so the code here calling them \"normal\" is *correct* - but It\u0027s getting there through two layers of defensive programming that wouldn\u0027t be necessary if we just normalized:\n\n```\nif hist_dev \u003d\u003d cur_dev:\n    tiers.append((r, \u0027normal\u0027))\n```","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":569,"context_line":"            elif hist_dev \u003d\u003d none_dev:"},{"line_number":570,"context_line":"                tiers.append((r, \u0027urgent\u0027))"},{"line_number":571,"context_line":"            else:"},{"line_number":572,"context_line":"                tiers.append((r, \u0027important\u0027))"},{"line_number":573,"context_line":"        return tiers"},{"line_number":574,"context_line":""},{"line_number":575,"context_line":"    def get_part_old_nodes(self, part):"}],"source_content_type":"text/x-python","patch_set":3,"id":"a92373c2_3fdefdce","line":572,"updated":"2026-08-31 22:36:39.000000000","message":"the only callers of this method in 995331: obj: order replication/reconstructor jobs by ring-history severity | https://review.opendev.org/c/openstack/swift/+/995331 ONLY ever consider `severity !\u003d normal` - suggesting to me that this isn\u0027t even the right interface?\n\nI think we also need to be careful about assigning connotations; the interesting thing about a primary that observes `cur_dev !\u003d hist_dev !\u003d none_dev` in the reconstructor this needs to be a \"do not rebuild!\" situation because we actually want to allow the handoff node that notices it has a local part-replica which has recently moved to prioritize revert before the primaries try to rebuild it.\n\ni.e. what\u0027s \"important\" depends on if you\u0027re the primary or handoff node - to the ring it\u0027s all just \"recently moved\"\n\nOTOH I think almost everyone should jump to action to do what they can do to make their replica of any part who\u0027s recently failed as consistent as possible \n\n... to the point where we may find that after some number of cycles/hours we might replace an old none_dev entry in the history table once we think it\u0027s been repaired (or maybe an over abundance of caution is a good thing)","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":588,"context_line":"            return []"},{"line_number":589,"context_line":"        none_dev \u003d none_dev_id(self._dev_id_bytes)"},{"line_number":590,"context_line":"        result \u003d []"},{"line_number":591,"context_line":"        seen_ids \u003d set()"},{"line_number":592,"context_line":"        for r, r2p2d in enumerate(self._replica2part2dev_id):"},{"line_number":593,"context_line":"            if part \u003e\u003d len(r2p2d):"},{"line_number":594,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"c69b078d_67d3659c","line":591,"updated":"2026-08-31 22:36:39.000000000","message":"I would would have SWORE ring validation disallows this:\n\n```\n  if len(devs_for_part) !\u003d len(set(devs_for_part)):\n      raise RingValidationError(\n          \"The partition %s has been assigned to duplicate devices %r\")\n```","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"f6bd33a918fb509e0f61db1bfbd4c0b643a7c2ca","unresolved":true,"context_lines":[{"line_number":593,"context_line":"            if part \u003e\u003d len(r2p2d):"},{"line_number":594,"context_line":"                continue"},{"line_number":595,"context_line":"            if r \u003e\u003d len(history):"},{"line_number":596,"context_line":"                continue"},{"line_number":597,"context_line":"            hist_row \u003d history[r]"},{"line_number":598,"context_line":"            if part \u003e\u003d len(hist_row):"},{"line_number":599,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"98424c33_514da691","line":596,"updated":"2026-08-28 22:04:56.000000000","message":"maybe agent pointed out these appear overly defensive and untested\n\n  \u003e Are these shape fallbacks intended as forward compatibility for a future non-dense history representation? If so, _validate_history() currently prevents that path at load time. I think a new named section is the clearer evolution boundary; this section’s shape can then remain a strict invariant.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":593,"context_line":"            if part \u003e\u003d len(r2p2d):"},{"line_number":594,"context_line":"                continue"},{"line_number":595,"context_line":"            if r \u003e\u003d len(history):"},{"line_number":596,"context_line":"                continue"},{"line_number":597,"context_line":"            hist_row \u003d history[r]"},{"line_number":598,"context_line":"            if part \u003e\u003d len(hist_row):"},{"line_number":599,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"52f8a156_dedbdf3d","line":596,"in_reply_to":"98424c33_514da691","updated":"2026-08-31 22:36:39.000000000","message":"upon further reflection this is related to replica count changes - in particular there are some interesting choices about what (if anything) to write in history during a 3\u003d\u003e2 replica count change and if a \"fractional\" replica (e.g. 2.5) should be allowed to have 3 nodes of history for for a 2 replica part\n\nN.B. in this loop we\u0027re using the replica count of `r2p2d` as canonical - so the extra history if any would be wasted anyway!  i.e. I think truncation of history to ALWAYS match the shape `r2p2d` is going to be easier to reason about and maintain.  \n\nWhen *increasing* the size of `r2p2d` it\u0027s not obvious to me if you\u0027d want new replicas to be NONE_DEV instead of just \"current\" - none of `r2p2d` resizing makes sense outside of replicated policies.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":599,"context_line":"                continue"},{"line_number":600,"context_line":"            hist_dev \u003d hist_row[part]"},{"line_number":601,"context_line":"            if hist_dev \u003d\u003d r2p2d[part] or hist_dev \u003d\u003d none_dev:"},{"line_number":602,"context_line":"                continue"},{"line_number":603,"context_line":"            if hist_dev \u003e\u003d len(self._devs) or self._devs[hist_dev] is None:"},{"line_number":604,"context_line":"                continue"},{"line_number":605,"context_line":"            if hist_dev in seen_ids:"}],"source_content_type":"text/x-python","patch_set":3,"id":"b11c8028_5dcad67c","line":602,"updated":"2026-08-31 22:36:39.000000000","message":"ok, so this method is NOT trying to return \"a list of the nodes from history\" - but more specifically; other valid device targets not otherwise seen in the primary table.\n\nI guess that\u0027s what it means by `important` - the `normal` and `urgent` values are skipped!","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":602,"context_line":"                continue"},{"line_number":603,"context_line":"            if hist_dev \u003e\u003d len(self._devs) or self._devs[hist_dev] is None:"},{"line_number":604,"context_line":"                continue"},{"line_number":605,"context_line":"            if hist_dev in seen_ids:"},{"line_number":606,"context_line":"                continue"},{"line_number":607,"context_line":"            seen_ids.add(hist_dev)"},{"line_number":608,"context_line":"            result.append(dict(self._devs[hist_dev], index\u003dr))"}],"source_content_type":"text/x-python","patch_set":3,"id":"a715ee1f_c694ee24","line":605,"updated":"2026-08-31 22:36:39.000000000","message":"I\u0027m not sure what it would mean for history to show that different replicas of a part have at different times been assigned to the same device - but also I\u0027m not sure why the replica indexed list of history nodes wouldn\u0027t try to expose that?\n\nMaybe the only use-case we have either for the proxy and consistency engine is always/only going to be \"any other valid nodes I might want to check?\" - and making that a de-duplicated set simplifies every caller having to check for duplicates\n\n... but it seems like an unfortuante loss of information at the primary source.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"d37b7d75ef1ae56318ed3918f8c92bf19a95bf1b","unresolved":true,"context_lines":[{"line_number":605,"context_line":"            if hist_dev in seen_ids:"},{"line_number":606,"context_line":"                continue"},{"line_number":607,"context_line":"            seen_ids.add(hist_dev)"},{"line_number":608,"context_line":"            result.append(dict(self._devs[hist_dev], index\u003dr))"},{"line_number":609,"context_line":"        return result"},{"line_number":610,"context_line":""},{"line_number":611,"context_line":"    def get_part(self, account, container\u003dNone, obj\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5beb577b_7b5c7ffd","line":608,"updated":"2026-08-31 22:36:39.000000000","message":"N.B. `get_part_old_nodes` can never tell you about a `index\u003dr` that has recently failed/`none_dev` - only *replacement* devices.","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"}],"test/unit/common/ring/test_builder.py":[{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"fe34d1f998d1cad2a87200a29b4cdd83e95d707f","unresolved":true,"context_lines":[{"line_number":5731,"context_line":"        rb._history_replica2part2dev[0][0] \u003d ("},{"line_number":5732,"context_line":"            (rb._replica2part2dev[0][0] + 1) % 4)"},{"line_number":5733,"context_line":"        path \u003d os.path.join(self.testdir, \u0027roundtrip.ring.gz\u0027)"},{"line_number":5734,"context_line":"        rb.get_ring().save(path, format_version\u003d2)"},{"line_number":5735,"context_line":"        loaded \u003d ring.RingData.load(path)"},{"line_number":5736,"context_line":"        self.assertIsNotNone(loaded._history_replica2part2dev_id)"},{"line_number":5737,"context_line":"        for builder_row, disk_row in zip(rb._history_replica2part2dev,"}],"source_content_type":"text/x-python","patch_set":3,"id":"8f630f1c_71d8754e","line":5734,"range":{"start_line":5734,"start_character":33,"end_line":5734,"end_character":50},"updated":"2026-07-31 04:49:36.000000000","message":"This is completely off-topic, but I wonder when we should just default to format_version\u003d2?","commit_id":"62d27807a7de63dafb6796c43c2009741ab252ca"}]}