)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"6d19e1ca_b7aaf1b4","updated":"2025-04-09 16:04:01.000000000","message":"LGTM!","commit_id":"4bb2f20b64f46aa408d1511805bc02443103c409"}],"swift/common/middleware/s3api/s3api.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":159,"context_line":"    InternalError, MethodNotAllowed, S3ResponseBase, S3NotImplemented"},{"line_number":160,"context_line":"from swift.common.utils import get_logger, config_true_value, \\"},{"line_number":161,"context_line":"    config_positive_int_value, split_path, closing_if_possible, \\"},{"line_number":162,"context_line":"    list_from_csv, parse_header, checksum"},{"line_number":163,"context_line":"from swift.common.middleware.s3api.utils import Config"},{"line_number":164,"context_line":"from swift.common.middleware.s3api.acl_handlers import get_acl_handler"},{"line_number":165,"context_line":"from swift.common.registry import register_swift_info, \\"}],"source_content_type":"text/x-python","patch_set":2,"id":"1c56b1fd_d9f233ea","line":162,"updated":"2025-04-09 16:04:01.000000000","message":"it\u0027s a little weird to see this as a new import - I assume *somewhere* in common.mw.s3api we must have already been importing checksum","commit_id":"4bb2f20b64f46aa408d1511805bc02443103c409"}],"swift/common/utils/checksum.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"71407931a59a1930a83cc422e890f857c9028446","unresolved":true,"context_lines":[{"line_number":207,"context_line":"    impl \u003d _select_crc32c_impl()"},{"line_number":208,"context_line":"    if impl is crc32c_ref:"},{"line_number":209,"context_line":"        logger.warning(\u0027Using (slow) %s implementation for CRC32C; \u0027"},{"line_number":210,"context_line":"                       \u0027install ISA-L for faster checksums.\u0027 % impl.__name__)"},{"line_number":211,"context_line":"    else:"},{"line_number":212,"context_line":"        logger.info(\u0027Using %s implementation for CRC32C.\u0027 % impl.__name__)"}],"source_content_type":"text/x-python","patch_set":1,"id":"e03c2d3f_692511d0","line":210,"updated":"2025-04-07 17:13:02.000000000","message":"moving the warning to \"on-demand\" is the primary goal of this patch","commit_id":"fff996a40ad2772c28b6f4b6ea2042464991a6b6"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":207,"context_line":"    impl \u003d _select_crc32c_impl()"},{"line_number":208,"context_line":"    if impl is crc32c_ref:"},{"line_number":209,"context_line":"        logger.warning(\u0027Using (slow) %s implementation for CRC32C; \u0027"},{"line_number":210,"context_line":"                       \u0027install ISA-L for faster checksums.\u0027 % impl.__name__)"},{"line_number":211,"context_line":"    else:"},{"line_number":212,"context_line":"        logger.info(\u0027Using %s implementation for CRC32C.\u0027 % impl.__name__)"}],"source_content_type":"text/x-python","patch_set":1,"id":"835e4171_e7c529c3","line":210,"in_reply_to":"e03c2d3f_692511d0","updated":"2025-04-09 16:04:01.000000000","message":"the interpolation isn\u0027t obviously needed here given the conditional, but perhaps will be useful if the logging is refactored to include multiple dynamic implementation selection(s).","commit_id":"fff996a40ad2772c28b6f4b6ea2042464991a6b6"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"71407931a59a1930a83cc422e890f857c9028446","unresolved":true,"context_lines":[{"line_number":209,"context_line":"        logger.warning(\u0027Using (slow) %s implementation for CRC32C; \u0027"},{"line_number":210,"context_line":"                       \u0027install ISA-L for faster checksums.\u0027 % impl.__name__)"},{"line_number":211,"context_line":"    else:"},{"line_number":212,"context_line":"        logger.info(\u0027Using %s implementation for CRC32C.\u0027 % impl.__name__)"}],"source_content_type":"text/x-python","patch_set":1,"id":"aba59639_ec07d084","line":212,"updated":"2025-04-07 17:13:02.000000000","message":"this seemed like it might be \"nice to have\" as we\u0027re shaking out the deps on various impl providers","commit_id":"fff996a40ad2772c28b6f4b6ea2042464991a6b6"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":209,"context_line":"        logger.warning(\u0027Using (slow) %s implementation for CRC32C; \u0027"},{"line_number":210,"context_line":"                       \u0027install ISA-L for faster checksums.\u0027 % impl.__name__)"},{"line_number":211,"context_line":"    else:"},{"line_number":212,"context_line":"        logger.info(\u0027Using %s implementation for CRC32C.\u0027 % impl.__name__)"}],"source_content_type":"text/x-python","patch_set":1,"id":"a70a8ecf_41f4240b","line":212,"in_reply_to":"aba59639_ec07d084","updated":"2025-04-09 16:04:01.000000000","message":"could become a debug at some point, I don\u0027t actually want a bunch of different implementations if we have one that\u0027s good enough (i.e. fast AND portable)","commit_id":"fff996a40ad2772c28b6f4b6ea2042464991a6b6"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":194,"context_line":""},{"line_number":195,"context_line":"def crc32c(data\u003dNone, initial_value\u003d0):"},{"line_number":196,"context_line":"    return CRCHasher(\u0027crc32c\u0027,"},{"line_number":197,"context_line":"                     _select_crc32c_impl(),"},{"line_number":198,"context_line":"                     data\u003ddata,"},{"line_number":199,"context_line":"                     initial_value\u003dinitial_value)"},{"line_number":200,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"ca81b21e_20eebffa","line":197,"updated":"2025-04-09 16:04:01.000000000","message":"unrelated, but given the dynamic nature of python I think it\u0027s at least theoretically possible for `_select_crc32c_impl` to return a different value *here* then when it\u0027s called during `log_selected_implementation`\n\nI wonder if `_select_crc32c_impl` *should* be called as import side-effect to \"lock-in\" a global `selected_crc32c_impl \u003d _select_crc32c_impl`\n\nI don\u0027t think it would drastically change the nature of either the tests where we\u0027d\n\n```\nwith mock.patch(isa_l, None):\n    assert(crc_ref, _select_crc())\n```\n\nnor\n\n```\nwith mock.patch(selected_crc, isa_l):\n    assert(\u0027using isa-l\u0027, log_selected())\n```","commit_id":"4bb2f20b64f46aa408d1511805bc02443103c409"}],"test/unit/common/middleware/s3api/test_s3api.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":244,"context_line":"        self.assertEqual([(b\u0027s3api.test-metric:1|c\u0027, (\u00271.2.3.4\u0027, 8125))],"},{"line_number":245,"context_line":"                         client.sendto_calls)"},{"line_number":246,"context_line":""},{"line_number":247,"context_line":"    def test_init_logs_checksum_implementation(self):"},{"line_number":248,"context_line":"        with mock.patch(\u0027swift.common.middleware.s3api.s3api.get_logger\u0027,"},{"line_number":249,"context_line":"                        return_value\u003dself.logger), \\"},{"line_number":250,"context_line":"                mock.patch(\u0027swift.common.utils.checksum.crc32c_isal\u0027) \\"}],"source_content_type":"text/x-python","patch_set":2,"id":"0e995fc5_a3849a2e","line":247,"updated":"2025-04-09 16:04:01.000000000","message":"nice, that should do it.","commit_id":"4bb2f20b64f46aa408d1511805bc02443103c409"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":249,"context_line":"                        return_value\u003dself.logger), \\"},{"line_number":250,"context_line":"                mock.patch(\u0027swift.common.utils.checksum.crc32c_isal\u0027) \\"},{"line_number":251,"context_line":"                as mock_crc32c:"},{"line_number":252,"context_line":"            mock_crc32c.__name__ \u003d \u0027crc32c_isal\u0027"},{"line_number":253,"context_line":"            S3ApiMiddleware(None, {})"},{"line_number":254,"context_line":"        self.assertEqual("},{"line_number":255,"context_line":"            {\u0027info\u0027: [\u0027Using crc32c_isal implementation for CRC32C.\u0027]},"}],"source_content_type":"text/x-python","patch_set":2,"id":"75fcfc19_0d786b8a","line":252,"updated":"2025-04-09 16:04:01.000000000","message":"ok, well *now* I *have* to see what was getting logged w/o this.\n\n```\n    def test_init_logs_checksum_implementation(self):\n        with mock.patch(\u0027swift.common.middleware.s3api.s3api.get_logger\u0027,\n                        return_value\u003dself.logger), \\\n                mock.patch(\u0027swift.common.utils.checksum.crc32c_isal\u0027) \\\n                as mock_crc32c:\n            # mock_crc32c.__name__ \u003d \u0027crc32c_isal\u0027\n\u003e           S3ApiMiddleware(None, {})\n\nswift/test/unit/common/middleware/s3api/test_s3api.py:253: \n_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ \nswift/swift/common/middleware/s3api/s3api.py:303: in __init__\n    checksum.log_selected_implementation(self.logger)\nswift/swift/common/utils/checksum.py:212: in log_selected_implementation\n    logger.info(\u0027Using %s implementation for CRC32C.\u0027 % impl.__name__)\n_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ \n\nself \u003d \u003cMagicMock name\u003d\u0027crc32c_isal\u0027 id\u003d\u0027139893786667104\u0027\u003e, name \u003d \u0027__name__\u0027\n\n    def __getattr__(self, name):\n        if name in {\u0027_mock_methods\u0027, \u0027_mock_unsafe\u0027}:\n            raise AttributeError(name)\n        elif self._mock_methods is not None:\n            if name not in self._mock_methods or name in _all_magics:\n                raise AttributeError(\"Mock object has no attribute %r\" % name)\n        elif _is_magic(name):\n\u003e           raise AttributeError(name)\nE           AttributeError: __name__\n\n/usr/lib/python3.10/unittest/mock.py:645: AttributeError\n```\n\nNot what I was expecting!\n\n```\n\u003e\u003e\u003e from unittest import mock\n\u003e\u003e\u003e thing \u003d mock.MagicMock()\n\u003e\u003e\u003e thing\n\u003cMagicMock id\u003d\u0027140154101534624\u0027\u003e\n\u003e\u003e\u003e thing.__name__\nTraceback (most recent call last):\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\n  File \"/usr/lib/python3.10/unittest/mock.py\", line 645, in __getattr__\n    raise AttributeError(name)\nAttributeError: __name__\n\u003e\u003e\u003e a \u003d lambda: None\n\u003e\u003e\u003e a.__name__\n\u0027\u003clambda\u003e\u0027\n```","commit_id":"4bb2f20b64f46aa408d1511805bc02443103c409"}],"test/unit/common/utils/test_checksum.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"a6b24f0fdad7504eb51b5d171f502da711daec27","unresolved":true,"context_lines":[{"line_number":250,"context_line":""},{"line_number":251,"context_line":"    def test_crc32c_hasher_selects_ref_impl(self):"},{"line_number":252,"context_line":"        with mock.patch(\u0027swift.common.utils.checksum.crc32c_isal\u0027, None), \\"},{"line_number":253,"context_line":"                mock.patch(\u0027swift.common.utils.checksum.crc32c_kern\u0027, None):"},{"line_number":254,"context_line":"            self.assertIs(checksum.crc32c_ref, checksum.crc32c().crc_func)"},{"line_number":255,"context_line":"            checksum.log_selected_implementation(self.logger)"},{"line_number":256,"context_line":"        self.assertIn(\u0027Using (slow) crc32c_ref implementation for CRC32C; \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"d2b7219f_672a9403","line":253,"updated":"2025-04-09 16:04:01.000000000","message":"interesting!  no `__name__ \u003d\u003d` here because we\u0027re not actually replacing the `_ref` function with a mock - it\u0027s just *always* available in any environment that could possibly be running this test - so \"all\" we have to do is blat out the other implementations.","commit_id":"4bb2f20b64f46aa408d1511805bc02443103c409"}]}