)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b740554721cc2c8e9b923ed11aa7349cf5fe0e98","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"bee8d531_18049d7f","updated":"2025-02-05 16:32:10.000000000","message":"My main hesitation is that all of the tests seem to be using policy 0 -- could we get some tests with a mix of policies? It\u0027d be especially easy in test_listing_formats.py -- they can be arbitrary strings by that point.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dc15970aed6956c85bf29f820331efd5b1c13277","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"ee0a6dd1_a74fb62a","in_reply_to":"245183a2_6e488e97","updated":"2025-02-05 20:11:17.000000000","message":"We\u0027ve got a `@patch_policies` decorator that helps a lot -- see https://github.com/openstack/swift/blob/2.34.0/test/unit/account/test_backend.py#L991-L995 for example.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"f2aa6874dc0842a603b5e28a60c165084955efed","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"245183a2_6e488e97","in_reply_to":"9adf2740_1e39164d","updated":"2025-02-05 19:37:49.000000000","message":"I\u0027ve added tests to test_listing_formats.py as suggested. Testing the handling inside the account server seems to require adding more actual storage policies (unless specific tests are added that patch required internal things) - do you think these are necessary as well, and if so, how would you handle these?","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"fb9fe97c7315e85bfd88efcfa22324301584aa81","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"9adf2740_1e39164d","in_reply_to":"bee8d531_18049d7f","updated":"2025-02-05 19:04:23.000000000","message":"Makes sense, thanks. I\u0027ll write some tests that use a mix of policies.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"d8f6a01ac288f619fb161169ae30b8e54aa4e8dd","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":3,"id":"80cdddd3_eca89e63","in_reply_to":"ee0a6dd1_a74fb62a","updated":"2025-02-05 22:17:30.000000000","message":"Now added `@patch_policies` with multiple policies checked to most of the updated tests.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"2cff0046_ac51ff7d","updated":"2025-02-11 22:49:59.000000000","message":"I threw some follow-ups in https://review.opendev.org/c/openstack/swift/+/941301 but this all looks good to me. Thanks Callum!","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}],"swift/account/backend.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b740554721cc2c8e9b923ed11aa7349cf5fe0e98","unresolved":true,"context_lines":[{"line_number":424,"context_line":"                        query_args)"},{"line_number":425,"context_line":"                except sqlite3.OperationalError as err:"},{"line_number":426,"context_line":"                    # If the storage policy column is not available,"},{"line_number":427,"context_line":"                    # the database has not been migrated to the new schema"},{"line_number":428,"context_line":"                    # with storage_policy_index. Re-run the query with"},{"line_number":429,"context_line":"                    # storage_policy_index set to a specific value"},{"line_number":430,"context_line":"                    # so that the caller knows how to handle this case."}],"source_content_type":"text/x-python","patch_set":3,"id":"b6977182_52567fbb","line":427,"range":{"start_line":427,"start_character":22,"end_line":427,"end_character":74},"updated":"2025-02-05 16:32:10.000000000","message":"I was going to ask where the diff is to do the migration, but it looks like we\u0027ve [already had it for ages!](https://github.com/openstack/swift/commit/00a162c4d42a0f7813058d2ae5c3208b4f7a3157)\n\nYeah, absolutely, let\u0027s include it!","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"fb9fe97c7315e85bfd88efcfa22324301584aa81","unresolved":true,"context_lines":[{"line_number":424,"context_line":"                        query_args)"},{"line_number":425,"context_line":"                except sqlite3.OperationalError as err:"},{"line_number":426,"context_line":"                    # If the storage policy column is not available,"},{"line_number":427,"context_line":"                    # the database has not been migrated to the new schema"},{"line_number":428,"context_line":"                    # with storage_policy_index. Re-run the query with"},{"line_number":429,"context_line":"                    # storage_policy_index set to a specific value"},{"line_number":430,"context_line":"                    # so that the caller knows how to handle this case."}],"source_content_type":"text/x-python","patch_set":3,"id":"6e2e4750_aff1bb22","line":427,"range":{"start_line":427,"start_character":22,"end_line":427,"end_character":74},"in_reply_to":"b6977182_52567fbb","updated":"2025-02-05 19:04:23.000000000","message":"Thank you for your positive feedback Tim, I appreciate the quick response.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dc15970aed6956c85bf29f820331efd5b1c13277","unresolved":true,"context_lines":[{"line_number":433,"context_line":"                    # then this special handling can be removed."},{"line_number":434,"context_line":"                    if \"no such column: storage_policy_index\" in str(err):"},{"line_number":435,"context_line":"                        curs \u003d conn.execute("},{"line_number":436,"context_line":"                            query.format(storage_policy_index\u003d\"-1\"),"},{"line_number":437,"context_line":"                            query_args)"},{"line_number":438,"context_line":"                    else:"},{"line_number":439,"context_line":"                        raise"}],"source_content_type":"text/x-python","patch_set":3,"id":"47bd9d35_b6100184","line":436,"range":{"start_line":436,"start_character":63,"end_line":436,"end_character":65},"updated":"2025-02-05 20:11:17.000000000","message":"Come to think of it, we could default to 0 here, like would happen once `_migrate_add_storage_policy_index` runs.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"a1a868c0e4f32d127d218619bf0393f7af3fd971","unresolved":true,"context_lines":[{"line_number":433,"context_line":"                    # then this special handling can be removed."},{"line_number":434,"context_line":"                    if \"no such column: storage_policy_index\" in str(err):"},{"line_number":435,"context_line":"                        curs \u003d conn.execute("},{"line_number":436,"context_line":"                            query.format(storage_policy_index\u003d\"-1\"),"},{"line_number":437,"context_line":"                            query_args)"},{"line_number":438,"context_line":"                    else:"},{"line_number":439,"context_line":"                        raise"}],"source_content_type":"text/x-python","patch_set":3,"id":"7518328f_c9bed15b","line":436,"range":{"start_line":436,"start_character":63,"end_line":436,"end_character":65},"in_reply_to":"1d4f86b5_851c895b","updated":"2025-02-05 22:27:28.000000000","message":"Thank you Tim and Matthew for explaining how the migration is handled. I\u0027ve now changed the patch so the storage policy index is set to 0 (for default policy) for pre-object storage policy databases.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"175f54114a5b93979d9386e70fafc96785078179","unresolved":true,"context_lines":[{"line_number":433,"context_line":"                    # then this special handling can be removed."},{"line_number":434,"context_line":"                    if \"no such column: storage_policy_index\" in str(err):"},{"line_number":435,"context_line":"                        curs \u003d conn.execute("},{"line_number":436,"context_line":"                            query.format(storage_policy_index\u003d\"-1\"),"},{"line_number":437,"context_line":"                            query_args)"},{"line_number":438,"context_line":"                    else:"},{"line_number":439,"context_line":"                        raise"}],"source_content_type":"text/x-python","patch_set":3,"id":"1d4f86b5_851c895b","line":436,"range":{"start_line":436,"start_character":63,"end_line":436,"end_character":65},"in_reply_to":"47bd9d35_b6100184","updated":"2025-02-05 21:21:42.000000000","message":"Yeah pre-storage policies we only had a single object ring, which in essence is always index 0. So pre storage policies we should just default it to 0. Because we\u0027ll basically always have 1.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b740554721cc2c8e9b923ed11aa7349cf5fe0e98","unresolved":true,"context_lines":[{"line_number":473,"context_line":"                            delim_force_gte \u003d True"},{"line_number":474,"context_line":"                        dir_name \u003d name[:end + len(delimiter)]"},{"line_number":475,"context_line":"                        if dir_name !\u003d orig_marker:"},{"line_number":476,"context_line":"                            results.append([dir_name, 0, 0, \u00270\u0027, 0, 1])"},{"line_number":477,"context_line":"                        curs.close()"},{"line_number":478,"context_line":"                        break"},{"line_number":479,"context_line":"                    results.append(row)"}],"source_content_type":"text/x-python","patch_set":3,"id":"d95ce919_add85702","line":476,"range":{"start_line":476,"start_character":65,"end_line":476,"end_character":66},"updated":"2025-02-05 16:32:10.000000000","message":"I\u0027m torn about whether to do 0 or -1, but either way I suppose the consumer is expected to ignore it.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dc15970aed6956c85bf29f820331efd5b1c13277","unresolved":true,"context_lines":[{"line_number":473,"context_line":"                            delim_force_gte \u003d True"},{"line_number":474,"context_line":"                        dir_name \u003d name[:end + len(delimiter)]"},{"line_number":475,"context_line":"                        if dir_name !\u003d orig_marker:"},{"line_number":476,"context_line":"                            results.append([dir_name, 0, 0, \u00270\u0027, 0, 1])"},{"line_number":477,"context_line":"                        curs.close()"},{"line_number":478,"context_line":"                        break"},{"line_number":479,"context_line":"                    results.append(row)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ebcbbbd5_5689f728","line":476,"range":{"start_line":476,"start_character":65,"end_line":476,"end_character":66},"in_reply_to":"3fd2a4b9_714c832b","updated":"2025-02-05 20:11:17.000000000","message":"I was thinking of it as an \"unknown\" / \"not applicable\" value. FWIW, anything that pre-dated storage policies ends up as storage policy 0 (which [should always be defined](https://github.com/openstack/swift/blob/2.34.0/swift/common/storage_policy.py#L771-L778)).","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"fb9fe97c7315e85bfd88efcfa22324301584aa81","unresolved":true,"context_lines":[{"line_number":473,"context_line":"                            delim_force_gte \u003d True"},{"line_number":474,"context_line":"                        dir_name \u003d name[:end + len(delimiter)]"},{"line_number":475,"context_line":"                        if dir_name !\u003d orig_marker:"},{"line_number":476,"context_line":"                            results.append([dir_name, 0, 0, \u00270\u0027, 0, 1])"},{"line_number":477,"context_line":"                        curs.close()"},{"line_number":478,"context_line":"                        break"},{"line_number":479,"context_line":"                    results.append(row)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fd2a4b9_714c832b","line":476,"range":{"start_line":476,"start_character":65,"end_line":476,"end_character":66},"in_reply_to":"d95ce919_add85702","updated":"2025-02-05 19:04:23.000000000","message":"My intention was for `-1` to mean \"storage policies not supported\". We could do another value for \"ignore me\", but since the consumer already looks for another value for `is_dir`, I didn\u0027t think it was necessary.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"d8f6a01ac288f619fb161169ae30b8e54aa4e8dd","unresolved":true,"context_lines":[{"line_number":473,"context_line":"                            delim_force_gte \u003d True"},{"line_number":474,"context_line":"                        dir_name \u003d name[:end + len(delimiter)]"},{"line_number":475,"context_line":"                        if dir_name !\u003d orig_marker:"},{"line_number":476,"context_line":"                            results.append([dir_name, 0, 0, \u00270\u0027, 0, 1])"},{"line_number":477,"context_line":"                        curs.close()"},{"line_number":478,"context_line":"                        break"},{"line_number":479,"context_line":"                    results.append(row)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5ed3a520_1d42e86d","line":476,"range":{"start_line":476,"start_character":65,"end_line":476,"end_character":66},"in_reply_to":"ebcbbbd5_5689f728","updated":"2025-02-05 22:17:30.000000000","message":"Updated the handling as described. `storage_policy_index` is now set to 0 for pre-storage policy databases. `-1` is now used as the placeholder value for the `is_dir` case, though this won\u0027t actually be looked at.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":367,"context_line":"        :param allow_reserved: exclude names with reserved-byte by default"},{"line_number":368,"context_line":""},{"line_number":369,"context_line":"        :returns: list of tuples of (name, object_count, bytes_used,"},{"line_number":370,"context_line":"                  put_timestamp, storage_policy_index, 0)"},{"line_number":371,"context_line":"        \"\"\""},{"line_number":372,"context_line":"        delim_force_gte \u003d False"},{"line_number":373,"context_line":"        if reverse:"}],"source_content_type":"text/x-python","patch_set":5,"id":"2b8edaf0_2ea8024b","line":370,"range":{"start_line":370,"start_character":53,"end_line":370,"end_character":56},"updated":"2025-02-11 22:49:59.000000000","message":"Off-topic: I wonder why we do that... maybe it\u0027s supposed to be analogous to [the `deleted` flag coming out of `list_objects_iter`](https://github.com/openstack/swift/blob/2.34.0/swift/container/backend.py#L1128-L1129)?\n\nIt\u0027s _old_, too; goes back to when this was first split out from db.py: https://github.com/openstack/swift/commit/d4b024ad\n\nOh! This is lying to us: it **isn\u0027t** always 0; it\u0027s an `is_subdir` flag!","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}],"swift/account/reaper.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":266,"context_line":"            while containers:"},{"line_number":267,"context_line":"                try:"},{"line_number":268,"context_line":"                    for (container, _junk, _junk, _junk, _junk,"},{"line_number":269,"context_line":"                         _junk) in containers:"},{"line_number":270,"context_line":"                        this_shard \u003d ("},{"line_number":271,"context_line":"                            int(md5(container.encode(\u0027utf-8\u0027),"},{"line_number":272,"context_line":"                                    usedforsecurity\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":5,"id":"339d1d9e_c432e376","line":269,"updated":"2025-02-11 22:49:59.000000000","message":"nit: Maybe at this point we want to just say\n```\nfor item in containers:\n    container \u003d item[0]\n```","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}],"swift/account/utils.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b740554721cc2c8e9b923ed11aa7349cf5fe0e98","unresolved":true,"context_lines":[{"line_number":99,"context_line":"            # (so the storage policy is unknown at this stage)."},{"line_number":100,"context_line":"            if storage_policy_index !\u003d -1:"},{"line_number":101,"context_line":"                container[\u0027storage_policy\u0027] \u003d ("},{"line_number":102,"context_line":"                    POLICIES[storage_policy_index].name"},{"line_number":103,"context_line":"                )"},{"line_number":104,"context_line":"            data.append(container)"},{"line_number":105,"context_line":"    if response_content_type.endswith(\u0027/xml\u0027):"}],"source_content_type":"text/x-python","patch_set":3,"id":"054df0ab_93866929","line":102,"updated":"2025-02-05 16:32:10.000000000","message":"This could raise a `KeyError` if this node has an out-of-date swift.conf -- better as\n```\nif storage_policy_index in POLICIES:\n```","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"fb9fe97c7315e85bfd88efcfa22324301584aa81","unresolved":true,"context_lines":[{"line_number":99,"context_line":"            # (so the storage policy is unknown at this stage)."},{"line_number":100,"context_line":"            if storage_policy_index !\u003d -1:"},{"line_number":101,"context_line":"                container[\u0027storage_policy\u0027] \u003d ("},{"line_number":102,"context_line":"                    POLICIES[storage_policy_index].name"},{"line_number":103,"context_line":"                )"},{"line_number":104,"context_line":"            data.append(container)"},{"line_number":105,"context_line":"    if response_content_type.endswith(\u0027/xml\u0027):"}],"source_content_type":"text/x-python","patch_set":3,"id":"73e42bfc_d45de1d3","line":102,"in_reply_to":"054df0ab_93866929","updated":"2025-02-05 19:04:23.000000000","message":"How is Swift normally expected to behave when `POLICIES` is empty? My worry with adding `if storage_policy_index in POLICIES` here is that introduces the possibility that some containers will have `storage_policy` returned and others will not, depending on whether the storage policy is defined in `swift.conf`.\n\nDo you think setting `storage_policy` to `None` would be better in that case?","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":false,"context_lines":[{"line_number":99,"context_line":"            # (so the storage policy is unknown at this stage)."},{"line_number":100,"context_line":"            if storage_policy_index !\u003d -1:"},{"line_number":101,"context_line":"                container[\u0027storage_policy\u0027] \u003d ("},{"line_number":102,"context_line":"                    POLICIES[storage_policy_index].name"},{"line_number":103,"context_line":"                )"},{"line_number":104,"context_line":"            data.append(container)"},{"line_number":105,"context_line":"    if response_content_type.endswith(\u0027/xml\u0027):"}],"source_content_type":"text/x-python","patch_set":3,"id":"0b668680_0875a7b2","line":102,"in_reply_to":"60b443b3_c6661c52","updated":"2025-02-11 22:49:59.000000000","message":"Done","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dc15970aed6956c85bf29f820331efd5b1c13277","unresolved":true,"context_lines":[{"line_number":99,"context_line":"            # (so the storage policy is unknown at this stage)."},{"line_number":100,"context_line":"            if storage_policy_index !\u003d -1:"},{"line_number":101,"context_line":"                container[\u0027storage_policy\u0027] \u003d ("},{"line_number":102,"context_line":"                    POLICIES[storage_policy_index].name"},{"line_number":103,"context_line":"                )"},{"line_number":104,"context_line":"            data.append(container)"},{"line_number":105,"context_line":"    if response_content_type.endswith(\u0027/xml\u0027):"}],"source_content_type":"text/x-python","patch_set":3,"id":"f564901a_0eae0b95","line":102,"in_reply_to":"73e42bfc_d45de1d3","updated":"2025-02-05 20:11:17.000000000","message":"`POLICIES` should never be empty, but you\u0027re right that you could get a mix of items with storage policy info and items without.\n\nThere are other places where [we ignore unknown policy info](https://github.com/openstack/swift/blob/2.34.0/swift/account/utils.py#L59-L63) -- as a result clients can\u0027t entirely expect that the sum of `X-Account-Storage-Policy-*-Bytes-Used` will equal `X-Account-Bytes-Used`, for example.\n\n`None` could work, too, though. In practice, I\u0027d expect a client wanting to use this new data would do something like\n```\nfor item in listing:\n    policy \u003d item.get(\u0027storage_policy\u0027)\n    ...\n```\nso the difference should be minimal. I guess there\u0027s something to be said for indicating that \"yes, I know about this feature, but I don\u0027t know about this policy\"","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"d8f6a01ac288f619fb161169ae30b8e54aa4e8dd","unresolved":true,"context_lines":[{"line_number":99,"context_line":"            # (so the storage policy is unknown at this stage)."},{"line_number":100,"context_line":"            if storage_policy_index !\u003d -1:"},{"line_number":101,"context_line":"                container[\u0027storage_policy\u0027] \u003d ("},{"line_number":102,"context_line":"                    POLICIES[storage_policy_index].name"},{"line_number":103,"context_line":"                )"},{"line_number":104,"context_line":"            data.append(container)"},{"line_number":105,"context_line":"    if response_content_type.endswith(\u0027/xml\u0027):"}],"source_content_type":"text/x-python","patch_set":3,"id":"60b443b3_c6661c52","line":102,"in_reply_to":"f564901a_0eae0b95","updated":"2025-02-05 22:17:30.000000000","message":"Thanks for your comments. I have updated it to:\n\n* Set `storage_policy_index` to 0 for pre-storage policy database, so account listings will return the default storage policy in that case.\n* Add `storage_policy_index in POLICIES` to the check so that the storage policy is not added if the name cannot be found. \n\nI\u0027ve also updated the comments to better document the expected behaviour.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":103,"context_line":"            # everything is configured correctly, but clients are"},{"line_number":104,"context_line":"            # expected to be able to handle this case regardless."},{"line_number":105,"context_line":"            if ("},{"line_number":106,"context_line":"                storage_policy_index \u003e\u003d 0"},{"line_number":107,"context_line":"                and storage_policy_index in POLICIES"},{"line_number":108,"context_line":"            ):"},{"line_number":109,"context_line":"                container[\u0027storage_policy\u0027] \u003d ("},{"line_number":110,"context_line":"                    POLICIES[storage_policy_index].name"}],"source_content_type":"text/x-python","patch_set":5,"id":"ee734fd9_3e02ab11","line":107,"range":{"start_line":106,"start_character":16,"end_line":107,"end_character":19},"updated":"2025-02-11 22:49:59.000000000","message":"nit: Not necessary -- `-1` will never be in `POLICIES`.","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}],"swift/common/middleware/listing_formats.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"b740554721cc2c8e9b923ed11aa7349cf5fe0e98","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":87,"context_line":"            for field in (\u0027storage_policy\u0027,):"},{"line_number":88,"context_line":"                if field in record:"},{"line_number":89,"context_line":"                    SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":90,"context_line":"        sub.tail \u003d \u0027\\n\u0027"},{"line_number":91,"context_line":"    return to_xml(doc)"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"8e12c82f_b703d0e9","line":89,"updated":"2025-02-05 16:32:10.000000000","message":"Right, so an upgraded account-server and a not-yet-upgraded proxy-server can lead to clients getting the `storage_policy` info for json responses but not xml ones -- but that should be fine; not really any different than if you were only talking to not-yet-upgraded servers.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"dc15970aed6956c85bf29f820331efd5b1c13277","unresolved":false,"context_lines":[{"line_number":86,"context_line":"                SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":87,"context_line":"            for field in (\u0027storage_policy\u0027,):"},{"line_number":88,"context_line":"                if field in record:"},{"line_number":89,"context_line":"                    SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":90,"context_line":"        sub.tail \u003d \u0027\\n\u0027"},{"line_number":91,"context_line":"    return to_xml(doc)"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"74774934_dfd22b78","line":89,"in_reply_to":"7191896c_98e2bd75","updated":"2025-02-05 20:11:17.000000000","message":"It could all be in one server, but even then you\u0027d have to worry a little about service-restart order. Generally you have 10s to 1,000s of servers, so there\u0027s always some version skew during upgrades. But again, I think this is as smooth an upgrade story as we could hope for.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"fb9fe97c7315e85bfd88efcfa22324301584aa81","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":87,"context_line":"            for field in (\u0027storage_policy\u0027,):"},{"line_number":88,"context_line":"                if field in record:"},{"line_number":89,"context_line":"                    SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":90,"context_line":"        sub.tail \u003d \u0027\\n\u0027"},{"line_number":91,"context_line":"    return to_xml(doc)"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"7191896c_98e2bd75","line":89,"in_reply_to":"8e12c82f_b703d0e9","updated":"2025-02-05 19:04:23.000000000","message":"Interesting, I wasn\u0027t aware those ran via different servers (not that experienced with Swift internals yet). It would be one thing if we were **removing** fields, but in this case we are adding a new one, and XML is similar to JSON in that there\u0027s no strict ordering of elements, so I think it should be fine as well.","commit_id":"0556b8977a8d4cd06a02fc25f9846b73c39551ec"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"175f54114a5b93979d9386e70fafc96785078179","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":87,"context_line":"            for field in (\u0027storage_policy\u0027,):"},{"line_number":88,"context_line":"                if field in record:"},{"line_number":89,"context_line":"                    SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":90,"context_line":"        sub.tail \u003d \u0027\\n\u0027"},{"line_number":91,"context_line":"    return to_xml(doc)"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"ddb22842_d8fb4876","line":89,"updated":"2025-02-05 21:21:42.000000000","message":"Can we just roll this up into the previous loop?\n\n```\nfor field in (\u0027name\u0027, \u0027count\u0027, \u0027bytes\u0027, \u0027last_modified\u0027, \u0027storage_policy\u0027):\n    if field in record:\n        SubElement(sub, field).text \u003d str(record.pop(field))\n```\n\nOr if there is a chance that we\u0027re missing things but we still want the sub elements but with a default\n\n```\nSubElement(sub, field).text \u003d str(record.pop(field, 0))\n```","commit_id":"098309193a2a9ae07a386522925c3502813c1509"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":87,"context_line":"            for field in (\u0027storage_policy\u0027,):"},{"line_number":88,"context_line":"                if field in record:"},{"line_number":89,"context_line":"                    SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":90,"context_line":"        sub.tail \u003d \u0027\\n\u0027"},{"line_number":91,"context_line":"    return to_xml(doc)"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"fcd975ef_e0713e1a","line":89,"in_reply_to":"90dc4600_bc224705","updated":"2025-02-11 22:49:59.000000000","message":"Yeah, I like the required/optional separation. And I agree that we shouldn\u0027t be including the field in XML responses if we don\u0027t have it in JSON.","commit_id":"098309193a2a9ae07a386522925c3502813c1509"},{"author":{"_account_id":36393,"name":"Callum Dickinson","email":"callum.dickinson@catalystcloud.nz","username":"Callum027","status":"Catalyst Cloud"},"change_message_id":"c8d07ff352912aae78123513faca513679aa581b","unresolved":true,"context_lines":[{"line_number":86,"context_line":"                SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":87,"context_line":"            for field in (\u0027storage_policy\u0027,):"},{"line_number":88,"context_line":"                if field in record:"},{"line_number":89,"context_line":"                    SubElement(sub, field).text \u003d str(record.pop(field))"},{"line_number":90,"context_line":"        sub.tail \u003d \u0027\\n\u0027"},{"line_number":91,"context_line":"    return to_xml(doc)"},{"line_number":92,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"90dc4600_bc224705","line":89,"in_reply_to":"ddb22842_d8fb4876","updated":"2025-02-05 21:34:13.000000000","message":"The reason why I didn\u0027t do this is because I considered the other attributes handled in the first loop required, while `storage_policy` explicitly should be treated as something that could not be returned. Making it a separate loop was the \"cleanest\" way of handling this, I found.\n\nBased on Tim\u0027s earlier comments, not creating the `\u003cstorage_policy\u003e` element if it wasn\u0027t returned by the account server seems like the way to go to me, but I can make an empty element if that makes more sense. The upcoming patch I will upload shortly will set the storage policy index to `0` for pre-storage policy databases on the account server end, so I don\u0027t think setting a default value here (for XML only) is a good idea.","commit_id":"098309193a2a9ae07a386522925c3502813c1509"}],"test/functional/swift_test_client.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":571,"context_line":"                for x in tree.getElementsByTagName(\u0027container\u0027):"},{"line_number":572,"context_line":"                    cont \u003d {}"},{"line_number":573,"context_line":"                    for key in [\u0027name\u0027, \u0027count\u0027, \u0027bytes\u0027, \u0027last_modified\u0027,"},{"line_number":574,"context_line":"                                \u0027storage_policy\u0027]:"},{"line_number":575,"context_line":"                        cont[key] \u003d x.getElementsByTagName(key)[0].\\"},{"line_number":576,"context_line":"                            childNodes[0].nodeValue"},{"line_number":577,"context_line":"                    conts.append(cont)"}],"source_content_type":"text/x-python","patch_set":5,"id":"64812253_97a49f4d","line":574,"updated":"2025-02-11 22:49:59.000000000","message":"Kind of a shame that there aren\u0027t any tests that actually *look at this*, but that seems to be the case for the `x-account-storage-policy-*` headers, too...","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}],"test/unit/account/test_utils.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":262,"context_line":"            \u0027storage_policy\u0027: POLICIES[1].name,"},{"line_number":263,"context_line":"        }]"},{"line_number":264,"context_line":"        self.assertEqual(sorted(json.dumps(expected).encode(\u0027ascii\u0027)),"},{"line_number":265,"context_line":"                         sorted(resp.body))"},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"    @patch_policies([StoragePolicy(0, \u0027zero\u0027, is_default\u003dTrue),"},{"line_number":268,"context_line":"                     StoragePolicy(1, \u0027one\u0027, is_default\u003dFalse)])"}],"source_content_type":"text/x-python","patch_set":5,"id":"d8a4b069_5da98654","line":265,"range":{"start_line":265,"start_character":25,"end_line":265,"end_character":42},"updated":"2025-02-11 22:49:59.000000000","message":"Wait a minute... if I throw in a `self.fail(sorted(resp.body))`:\n```\n\u003e       self.fail(sorted(resp.body))\nE       AssertionError: [32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 34, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 45, 45, 45, 45, 45, 45, 46, 46, 46, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 49, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 50, 53, 53, 53, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 58, 84, 84, 84, 91, 93, 95, 95, 95, 95, 95, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 98, 98, 98, 98, 98, 99, 99, 99, 99, 99, 100, 100, 100, 100, 100, 100, 101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 101, 102, 102, 102, 102, 103, 103, 105, 105, 105, 105, 105, 105, 105, 105, 108, 108, 108, 108, 108, 109, 109, 109, 109, 109, 109, 110, 110, 110, 110, 110, 110, 110, 111, 111, 111, 111, 111, 111, 111, 111, 111, 111, 111, 111, 111, 111, 112, 112, 114, 114, 114, 114, 115, 115, 115, 115, 115, 115, 115, 115, 116, 116, 116, 116, 116, 116, 116, 116, 116, 116, 116, 117, 117, 117, 121, 121, 121, 121, 121, 122, 122, 123, 123, 123, 125, 125, 125]\n```\nI don\u0027t think this is really going to give us the most useful errors if some other change breaks the test.","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":true,"context_lines":[{"line_number":335,"context_line":"            \u0027storage_policy\u0027: POLICIES[1].name,"},{"line_number":336,"context_line":"        }]"},{"line_number":337,"context_line":"        self.assertEqual(sorted(json.dumps(expected).encode(\u0027ascii\u0027)),"},{"line_number":338,"context_line":"                         sorted(resp.body))"}],"source_content_type":"text/x-python","patch_set":5,"id":"1f25d227_2dd56f62","line":338,"updated":"2025-02-11 22:49:59.000000000","message":"Oh, but it\u0027s got precedent! Yuck!","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}],"test/unit/common/middleware/test_listing_formats.py":[{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"7b7df2d8a64ba344d5bd17ca251e7e5c78f2a540","unresolved":false,"context_lines":[{"line_number":25,"context_line":"from test.unit.common.middleware.helpers import FakeSwift"},{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"TEST_POLICIES \u003d (POLICIES[0].name, \u0027Policy-1\u0027)"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"class TestListingFormats(unittest.TestCase):"}],"source_content_type":"text/x-python","patch_set":5,"id":"dbba8984_a6e4e8ed","line":28,"range":{"start_line":28,"start_character":35,"end_line":28,"end_character":45},"updated":"2025-02-11 22:49:59.000000000","message":"Nice! You\u0027ve even got coverage for when the account-server knows about a policy but the proxy doesn\u0027t!","commit_id":"965cc2fcbc18c025639c61e945bc211bf9a2783b"}]}