)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"9cf859983d532de7779b7fbc5534ed4f76a8ed73","unresolved":true,"context_lines":[{"line_number":14,"context_line":"to the same generator that yielded a node. This would not happen when"},{"line_number":15,"context_line":"the generator was replaced, and as a result it was possible for a"},{"line_number":16,"context_line":"NodeIter to yield more nodes than intended, as demonstrated by the"},{"line_number":17,"context_line":"existing unit test."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"The fix is to create the internal generator once in the NodeIter"},{"line_number":20,"context_line":"constructor."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"9eb97e2c_48131196","line":17,"updated":"2022-10-27 17:40:10.000000000","message":"With some pathologically-bad code, I think you could have gotten every node in the ring:\n\n while True:\n     try:\n         node \u003d next(iter(node_iter))\n     except StopIteration:\n         break","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"94e9c01e943e415d6c811a9d7f84ab5f3856e213","unresolved":false,"context_lines":[{"line_number":14,"context_line":"to the same generator that yielded a node. This would not happen when"},{"line_number":15,"context_line":"the generator was replaced, and as a result it was possible for a"},{"line_number":16,"context_line":"NodeIter to yield more nodes than intended, as demonstrated by the"},{"line_number":17,"context_line":"existing unit test."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"The fix is to create the internal generator once in the NodeIter"},{"line_number":20,"context_line":"constructor."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"a184bdd5_15b8734c","line":17,"in_reply_to":"9eb97e2c_48131196","updated":"2022-10-28 13:33:55.000000000","message":"Ack","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"056fc50061f2d75ab63d8f0ac1c0944d060631b9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"65d33cf3_4031cc71","updated":"2022-10-27 17:37:39.000000000","message":"recheck\n\nprobe test failure should be fixed by https://review.opendev.org/c/openstack/swift/+/862758","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"a73a1c3afec013ab0185031a1d90965f2dfe383c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"7d4b2602_5ab5ae81","updated":"2022-10-28 13:35:22.000000000","message":"I\u0027ve squashed in my follow on patch, which is where I really wanted to get to: I need to add further checks on nodes for 529 rate control and wanted to get the logic consolidated first.","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"ea1594ab_39739185","updated":"2022-10-28 18:23:55.000000000","message":"so good cleanup/fxies/refactoring here - are we going to dump the back propogration of error limiting into nodes_left?","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"}],"swift/proxy/controllers/base.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"9cf859983d532de7779b7fbc5534ed4f76a8ed73","unresolved":true,"context_lines":[{"line_number":1693,"context_line":"                if not self.app.error_limited(node):"},{"line_number":1694,"context_line":"                    self.nodes_left -\u003d 1"},{"line_number":1695,"context_line":"                    if self.nodes_left \u003c\u003d 0:"},{"line_number":1696,"context_line":"                        return"},{"line_number":1697,"context_line":"        handoffs \u003d 0"},{"line_number":1698,"context_line":"        for node in self.handoff_iter:"},{"line_number":1699,"context_line":"            if not self.app.error_limited(node):"}],"source_content_type":"text/x-python","patch_set":1,"id":"aa96be67_58d046ee","line":1696,"updated":"2022-10-27 17:40:10.000000000","message":"I wonder if it\u0027d be better for us to rework this block like\n\n            node \u003d self.primary_nodes.pop(0)\n            if not self.app.error_limited(node):\n                self.nodes_left -\u003d 1\n                yield node\n                if self.app.error_limited(node):\n                    self.nodes_left +\u003d 1\n                elif self.nodes_left \u003c\u003d 0:\n                    return\n\n(and below, too). I think it\u0027d fix up the accounting in that unit test so we don\u0027t feel the need to comment on it.\n\nSide note: Do we have any unit tests that cover a node getting error limited while control\u0027s been returned to the caller? How does this interact with things like concurrent_gets, where you might have\n\n- t0: get a node, make a request\n- t1: no response; get another node, make another request\n- t2: first request responds 507, so the node gets error limited\n\nIt seems like the idea of this extra error_limited(node) check is to go out an extra node when *this request* caused the node to get error limited, but we have to make that decision at t1 while the request is still in-flight...","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"94e9c01e943e415d6c811a9d7f84ab5f3856e213","unresolved":false,"context_lines":[{"line_number":1693,"context_line":"                if not self.app.error_limited(node):"},{"line_number":1694,"context_line":"                    self.nodes_left -\u003d 1"},{"line_number":1695,"context_line":"                    if self.nodes_left \u003c\u003d 0:"},{"line_number":1696,"context_line":"                        return"},{"line_number":1697,"context_line":"        handoffs \u003d 0"},{"line_number":1698,"context_line":"        for node in self.handoff_iter:"},{"line_number":1699,"context_line":"            if not self.app.error_limited(node):"}],"source_content_type":"text/x-python","patch_set":1,"id":"1ed09d31_8d1189d4","line":1696,"in_reply_to":"aa96be67_58d046ee","updated":"2022-10-28 13:33:55.000000000","message":"rework - Done.\n\nThe post-hoc check of error_limited does seem wonky if the iterator is used in concurrent green threads. It is likely that often control will return to the iterator, and the node is checked, before the node has been used. There is therefore no guarantee that nodes that do become error_limited will influence nodes_left.\n\nI don\u0027t see an easy fix for that - it probably requires an explicit call back to the iterator from the node-user to indicate that nodes_left should be incremented. I don\u0027t want to tackle that in this patch.","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"9cf859983d532de7779b7fbc5534ed4f76a8ed73","unresolved":true,"context_lines":[{"line_number":1698,"context_line":"        for node in self.handoff_iter:"},{"line_number":1699,"context_line":"            if not self.app.error_limited(node):"},{"line_number":1700,"context_line":"                handoffs +\u003d 1"},{"line_number":1701,"context_line":"                self.log_handoffs(handoffs)"},{"line_number":1702,"context_line":"                yield node"},{"line_number":1703,"context_line":"                if not self.app.error_limited(node):"},{"line_number":1704,"context_line":"                    self.nodes_left -\u003d 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"8ca2c30a_f23b59d8","line":1701,"updated":"2022-10-27 17:40:10.000000000","message":"Ick -- you could also reset the handoff counting previously...","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"94e9c01e943e415d6c811a9d7f84ab5f3856e213","unresolved":false,"context_lines":[{"line_number":1698,"context_line":"        for node in self.handoff_iter:"},{"line_number":1699,"context_line":"            if not self.app.error_limited(node):"},{"line_number":1700,"context_line":"                handoffs +\u003d 1"},{"line_number":1701,"context_line":"                self.log_handoffs(handoffs)"},{"line_number":1702,"context_line":"                yield node"},{"line_number":1703,"context_line":"                if not self.app.error_limited(node):"},{"line_number":1704,"context_line":"                    self.nodes_left -\u003d 1"}],"source_content_type":"text/x-python","patch_set":1,"id":"18f39114_d25f8b88","line":1701,"in_reply_to":"8ca2c30a_f23b59d8","updated":"2022-10-28 13:33:55.000000000","message":"Ack","commit_id":"bf06e3bbddcee64b7f585b0711a7ca1df28bd651"}],"test/unit/proxy/controllers/test_base.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1579,"context_line":"        self.assertEqual(12, node_iter.nodes_left)"},{"line_number":1580,"context_line":"        for node in node_iter:"},{"line_number":1581,"context_line":"            nodes.append(node)"},{"line_number":1582,"context_line":"        self.assertEqual(17, len(nodes))"}],"source_content_type":"text/x-python","patch_set":2,"id":"9301d33e_9d218d83","side":"PARENT","line":1582,"updated":"2022-10-28 18:23:55.000000000","message":"ah, so this was a bug!  Since the decr doesn\u0027t happen until after we pull out the node, the reset of the iterator gives us an extra handoff.  sneaky.","commit_id":"8d541fed4ee12d8472c15a62049de6c14bc0b02a"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1549,"context_line":"        lines \u003d self.logger.get_lines_for_level(\u0027warning\u0027)"},{"line_number":1550,"context_line":"        self.assertFalse(lines)"},{"line_number":1551,"context_line":""},{"line_number":1552,"context_line":"    def test_iter_with_extra_handoffs(self):"},{"line_number":1553,"context_line":"        ring \u003d FakeRing(replicas\u003d3, max_more_nodes\u003d20)  # handoffs available"},{"line_number":1554,"context_line":"        policy \u003d StoragePolicy(0, \u0027zero\u0027, object_ring\u003dring)"},{"line_number":1555,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"b1852c64_36658191","line":1552,"updated":"2022-10-28 18:23:55.000000000","message":"this is nice, the new tests passes on the old code too","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1566,"context_line":"            if \u0027index\u0027 in node:"},{"line_number":1567,"context_line":"                primary_indexes.add(node[\u0027index\u0027])"},{"line_number":1568,"context_line":"                if node[\u0027index\u0027] \u003d\u003d 1:"},{"line_number":1569,"context_line":"                    self.app.error_limit(node, \u0027error limiting primary\u0027)"},{"line_number":1570,"context_line":"            else:"},{"line_number":1571,"context_line":"                handoff_indexes.append(node[\u0027handoff_index\u0027])"},{"line_number":1572,"context_line":"                if node[\u0027handoff_index\u0027] \u003d\u003d 2:"}],"source_content_type":"text/x-python","patch_set":2,"id":"8e9df653_3b13bdf1","line":1569,"updated":"2022-10-28 18:23:55.000000000","message":"error limit primary index 1","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1570,"context_line":"            else:"},{"line_number":1571,"context_line":"                handoff_indexes.append(node[\u0027handoff_index\u0027])"},{"line_number":1572,"context_line":"                if node[\u0027handoff_index\u0027] \u003d\u003d 2:"},{"line_number":1573,"context_line":"                    self.app.error_limit(node, \u0027error limiting handoff\u0027)"},{"line_number":1574,"context_line":"            count +\u003d 1"},{"line_number":1575,"context_line":"        self.assertEqual(count, 8)"},{"line_number":1576,"context_line":"        self.assertEqual(0, node_iter.primaries_left)"}],"source_content_type":"text/x-python","patch_set":2,"id":"c4ddafab_bd7c5200","line":1573,"updated":"2022-10-28 18:23:55.000000000","message":"and handoff limit 2","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1576,"context_line":"        self.assertEqual(0, node_iter.primaries_left)"},{"line_number":1577,"context_line":"        self.assertEqual(0, node_iter.nodes_left)"},{"line_number":1578,"context_line":"        self.assertEqual({0, 1, 2}, primary_indexes)"},{"line_number":1579,"context_line":"        self.assertEqual([0, 1, 2, 3, 4], handoff_indexes)"},{"line_number":1580,"context_line":"        lines \u003d self.logger.get_lines_for_level(\u0027warning\u0027)"},{"line_number":1581,"context_line":"        self.assertEqual([\u0027Handoff requested (4)\u0027, \u0027Handoff requested (5)\u0027],"},{"line_number":1582,"context_line":"                         lines)"}],"source_content_type":"text/x-python","patch_set":2,"id":"8ab3dfe9_c2af4f36","line":1579,"updated":"2022-10-28 18:23:55.000000000","message":"it\u0027s curious - we asked for six nodes and error limited two of them and ended up with eight total.","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1598,"context_line":"        self.assertEqual(count, 6)"},{"line_number":1599,"context_line":"        self.assertEqual(0, node_iter.primaries_left)"},{"line_number":1600,"context_line":"        self.assertEqual(0, node_iter.nodes_left)"},{"line_number":1601,"context_line":"        self.assertEqual({0, 2}, primary_indexes)"},{"line_number":1602,"context_line":"        self.assertEqual([0, 1, 3, 4], handoff_indexes)"},{"line_number":1603,"context_line":"        lines \u003d self.logger.get_lines_for_level(\u0027warning\u0027)"},{"line_number":1604,"context_line":"        self.assertEqual([\u0027Handoff requested (4)\u0027], lines)"}],"source_content_type":"text/x-python","patch_set":2,"id":"490c8ac9_67c87119","line":1601,"updated":"2022-10-28 18:23:55.000000000","message":"primary 1 is missing","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9cd612be90f68d4a5e931ad7d5db1c8ef538e2ed","unresolved":true,"context_lines":[{"line_number":1599,"context_line":"        self.assertEqual(0, node_iter.primaries_left)"},{"line_number":1600,"context_line":"        self.assertEqual(0, node_iter.nodes_left)"},{"line_number":1601,"context_line":"        self.assertEqual({0, 2}, primary_indexes)"},{"line_number":1602,"context_line":"        self.assertEqual([0, 1, 3, 4], handoff_indexes)"},{"line_number":1603,"context_line":"        lines \u003d self.logger.get_lines_for_level(\u0027warning\u0027)"},{"line_number":1604,"context_line":"        self.assertEqual([\u0027Handoff requested (4)\u0027], lines)"},{"line_number":1605,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"04eb2d13_12c8c511","line":1602,"updated":"2022-10-28 18:23:55.000000000","message":"handoff 2 is missing\n\n... but still get the same total six nodes we asked for","commit_id":"5e7eef3e44f478570f46d4456f432126bc481115"}]}