)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9686e79cf566691296d39670f14fcb8275487fea","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Tim Burke \u003ctim.burke@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2025-06-16 13:28:56 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Clarify why there\u0027s a ShardRange.__hash__"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: I4eff901aaa334c8a73cebfc578cea14d23e6c365"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"a5bb08ae_cfbcb1f8","line":7,"updated":"2025-07-30 21:33:58.000000000","message":"but this isn\u0027t *just* a doc-string update?  Can you clarify what\u0027s broken w/o this change?","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3400f716e27c36e44bd668afac16f44ad3b3f8c7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0f106b0f_63891923","updated":"2025-07-31 18:56:21.000000000","message":"With a better testing, I think the functional change is behaviorally equivalent to master:\n\n```\n\u003e\u003e\u003e from swift.common.utils import ShardRange\n\u003e\u003e\u003e sr1 \u003d ShardRange(\u0027a/c-1\u0027)\n\u003e\u003e\u003e sr2 \u003d ShardRange(\u0027a/c-2\u0027)\n\u003e\u003e\u003e hash(sr1)\n139656858470000\n\u003e\u003e\u003e hash(sr2)\n139656824218544\n\u003e\u003e\u003e len({sr1, sr2})\n2\n\u003e\u003e\u003e sr1 \u003d\u003d sr2\nTrue\n```\n\nthis change:\n\n```\n\u003e\u003e\u003e sr2 \u003d ShardRange(\u0027a/c-2\u0027)\n\u003e\u003e\u003e sr1 \u003d ShardRange(\u0027a/c-1\u0027)\n\u003e\u003e\u003e hash(sr1)\n8768383878105\n\u003e\u003e\u003e hash(sr2)\n8768383878096\n\u003e\u003e\u003e sr1 \u003d\u003d sr2\nTrue\n\u003e\u003e\u003e len({sr1, sr2})\n2`\n```\n\n... I think the comment is an improvement.  KUDOS.","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9686e79cf566691296d39670f14fcb8275487fea","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"0eead2e1_4ad754eb","updated":"2025-07-30 21:33:58.000000000","message":"maybe this is fine - I have no idea at a glance - I don\u0027t understand why it\u0027s more important than other things I could or should look at and think about.","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"84ea48724fc546c5542fb6614f279166c6b40901","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"5e3e15e2_81c477f4","in_reply_to":"0eead2e1_4ad754eb","updated":"2025-07-30 22:19:17.000000000","message":"I was hoping it\u0027d be small and not terribly controversial -- guess I misjudged. More broadly, though, I\u0027m tired of having https://review.opendev.org/q/owner:me+is:open only look back 4 months in that first page of 50. If I don\u0027t advocate for it, that\u0027ll never improve 😜","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3400f716e27c36e44bd668afac16f44ad3b3f8c7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"eb728038_b5ebfdfe","in_reply_to":"5e3e15e2_81c477f4","updated":"2025-07-31 18:56:21.000000000","message":"\u003e https://review.opendev.org/q/owner:me+is:open only look back 4 months\n\nfor me that goes back to 2024\n\n\u003e If I don\u0027t advocate for it, that\u0027ll never improve\n\nmaybe.  Personal anecdote: I rarely find complaining about how I\u0027d like other people to do things is as efficient at asking myself: \"What could *I* do that would steer me towards the outcomes I want?\"  If your goal is to have less changes in gerrit waiting for other people to tell you if they\u0027re useful - maybe write less \"trivial and hopefully not controversial\" patches and instead write ONLY \"absolutely essential and unavoidably necessary\" patches that are STILL \"as small and easy to review as I can reasonably make them\" for the next 4mo and see how many of those merge?\n\nbut that is there not here - *here* what might have helped is a test demonstrating how this achieves the same thing as id(self):\n\n```\n\u003e\u003e\u003e sr2 \u003d ShardRange(\u0027a/c-2\u0027)\n\u003e\u003e\u003e sr1 \u003d ShardRange(\u0027a/c-1\u0027)\n\u003e\u003e\u003e hash(sr1)\n8768383878105\n\u003e\u003e\u003e hash(sr2)\n8768383878096\n\u003e\u003e\u003e sr1 \u003d\u003d sr2\nTrue\n\u003e\u003e\u003e len({sr1, sr2})\n2`\n```\n\nwith a commit message that elaborates you find `__hash__ \u003d\u003d object.__hash__` to more obviously state \"we want the default hash behavior had we not overridden `__eq__`\" (which I guess somehow turns out NOT to include the default behavior of \"when you add a custom `__eq__` you can\u0027t use default `__hash__`\").","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"}],"swift/common/utils/__init__.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"9686e79cf566691296d39670f14fcb8275487fea","unresolved":true,"context_lines":[{"line_number":4342,"context_line":"    # but we are happy to use their hash-derived-from-id()."},{"line_number":4343,"context_line":"    # NB: this breaks Python\u0027s assumption that \"Hashable objects which compare"},{"line_number":4344,"context_line":"    # equal must have the same hash value.\""},{"line_number":4345,"context_line":"    __hash__ \u003d object.__hash__"},{"line_number":4346,"context_line":""},{"line_number":4347,"context_line":"    def __repr__(self):"},{"line_number":4348,"context_line":"        return \u0027%s\u003c%r to %r as of %s, (%d, %d) as of %s, %s as of %s\u003e\u0027 % ("}],"source_content_type":"text/x-python","patch_set":1,"id":"6c965771_d879f89d","line":4345,"updated":"2025-07-30 21:33:58.000000000","message":"\u003e making this mutable class hashable\n\nIIRC I think it\u0027s a key observation that a) we don\u0027t ever mutate instances of SR once we create them and b) we really only assert equivalency of different instances of SR in tests.  I could be mis-remembering - I haven\u0027t invested recently to deeply understand the history of this implementation.  Some kind of \"if ain\u0027t broke\" bias.\n\n```\n(nvidia) cgerrard@NVStation:~/Workspace/scratch/copyparty$ python\nPython 3.12.3 (main, Jun 18 2025, 17:59:45) [GCC 13.3.0] on linux\nType \"help\", \"copyright\", \"credits\" or \"license\" for more information.\n\u003e\u003e\u003e class A:\n...    ...\n... \n\u003e\u003e\u003e o \u003d A()\n\u003e\u003e\u003e id(o)\n134068836975392\n\u003e\u003e\u003e o.__hash__()\n8379302310962\n\u003e\u003e\u003e o2 \u003d A()\n\u003e\u003e\u003e id(o2)\n134068836982208\n\u003e\u003e\u003e o.__hash__()\n8379302310962\n\u003e\u003e\u003e \n```\n\nI *think* this is changing the behavior?  Is that on purpose?\n\n\u003e NB: this breaks Python\u0027s assumption that \"Hashable objects which compare equal must have the same hash value.\"\n\ne.g. if you use the identity function it\u0027s not\u0027s possible to get the same hash from two different objects - so therefore if they have the same hash they are the same object and therefore must be equivalent.","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3400f716e27c36e44bd668afac16f44ad3b3f8c7","unresolved":true,"context_lines":[{"line_number":4342,"context_line":"    # but we are happy to use their hash-derived-from-id()."},{"line_number":4343,"context_line":"    # NB: this breaks Python\u0027s assumption that \"Hashable objects which compare"},{"line_number":4344,"context_line":"    # equal must have the same hash value.\""},{"line_number":4345,"context_line":"    __hash__ \u003d object.__hash__"},{"line_number":4346,"context_line":""},{"line_number":4347,"context_line":"    def __repr__(self):"},{"line_number":4348,"context_line":"        return \u0027%s\u003c%r to %r as of %s, (%d, %d) as of %s, %s as of %s\u003e\u0027 % ("}],"source_content_type":"text/x-python","patch_set":1,"id":"33f273db_c1fe666a","line":4345,"in_reply_to":"437bef9a_8e41fb05","updated":"2025-07-31 18:56:21.000000000","message":"\u003e we don\u0027t ever mutate instances of SR once we create them\n\u003e That\u0027s not true [...] merging donors into an acceptor changes the acceptor\u0027s bounds\n\nmae culpa - apparently I did NOT \"RC\"\n\n\u003e So why\u0027d we implement it [`__eq__`] at all?\n\nI\u0027m guessing it\u0027s really hard in tests to spot check if one SR upper and lower is the same of another SR.  Perhaps a `self.assertShardRangeEqual` method would be better than an test-only object behavior.\n\n\u003e should we rip it [`__eq__`] out?\n\nI\u0027m not sure I\u0027d be against it without seeing a diff, but certainly wouldn\u0027t advocate that anyone should spend time on that unless it\u0027s causing a problem.  e.g. someone needs to write `set(shard_ranges)` to dedupe a list - in which case maybe a better `__hash__` function would be the easiest way for them to describe what they mean?  something like a `_tuple_for_eq_and_hash()`???\n\n\u003e why were we writing our own `__hash__` function at all?\n\nBecause we added `__eq__` and still wanted to use them as keys in a dict.\n\n\u003e every other place we define one, we have it based on hash(self.something).\n\nI think `def __hash__(self): return hash((self.a, self.b, self.c))` would essentially be a resolution of the existing comment.  Which was, I guess, trying to justify that \"this is simple and does what we need\"\n\n\u003e I think this is changing the behavior? Is that on purpose?\n\u003e Yep \n\nI\u0027m not so sure anymore - I think I was wrong, this snippet had a typo:\n\n```\n\u003e\u003e\u003e o \u003d A()\n\u003e\u003e\u003e id(o)\n134068836975392\n\u003e\u003e\u003e o.__hash__()\n8379302310962\n\u003e\u003e\u003e o2 \u003d A()\n\u003e\u003e\u003e id(o2)\n134068836982208\n\u003e\u003e\u003e o.__hash__()\n8379302310962\n```\n\n... I had intended to check `o2.__hash__()` and misinterpreted the results.\n\n```\n\u003e\u003e\u003e class A:\n...    def __eq__(self, other):\n...       return False\n... \n\u003e\u003e\u003e \n\u003e\u003e\u003e o \u003d A()\n\u003e\u003e\u003e hash(o)\nTraceback (most recent call last):\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\nTypeError: unhashable type: \u0027A\u0027\n\u003e\u003e\u003e object.__hash__(o)\n8096023007762\n\u003e\u003e\u003e id(o)\n129536368124192\n\u003e\u003e\u003e o2 \u003d A()\n\u003e\u003e\u003e object.__hash__(o2)\n8096023018248\n\u003e\u003e\u003e object.__hash__(o)\n8096023007762\n\u003e\u003e\u003e \n```\n\nApparently you can reuse the default `object.__hash__` implementation w/o also getting the \"unhashable type\" error.\n\nI think the change here is just a *different* \"different number\" - but not actually a different \"behavior\".  Every instance of ShardRange (equal or not) will have a different hash.  From master:\n\n```\n\u003e\u003e\u003e from swift.common.utils import ShardRange\n\u003e\u003e\u003e sr1 \u003d ShardRange(\u0027a/c-1\u0027)\n\u003e\u003e\u003e sr2 \u003d ShardRange(\u0027a/c-2\u0027)\n\u003e\u003e\u003e hash(sr1)\n139656858470000\n\u003e\u003e\u003e hash(sr2)\n139656824218544\n\u003e\u003e\u003e len({sr1, sr2})\n2\n\u003e\u003e\u003e sr1 \u003d\u003d sr2\nTrue\n```","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"84ea48724fc546c5542fb6614f279166c6b40901","unresolved":true,"context_lines":[{"line_number":4342,"context_line":"    # but we are happy to use their hash-derived-from-id()."},{"line_number":4343,"context_line":"    # NB: this breaks Python\u0027s assumption that \"Hashable objects which compare"},{"line_number":4344,"context_line":"    # equal must have the same hash value.\""},{"line_number":4345,"context_line":"    __hash__ \u003d object.__hash__"},{"line_number":4346,"context_line":""},{"line_number":4347,"context_line":"    def __repr__(self):"},{"line_number":4348,"context_line":"        return \u0027%s\u003c%r to %r as of %s, (%d, %d) as of %s, %s as of %s\u003e\u0027 % ("}],"source_content_type":"text/x-python","patch_set":1,"id":"437bef9a_8e41fb05","line":4345,"in_reply_to":"6c965771_d879f89d","updated":"2025-07-30 22:19:17.000000000","message":"\u003e a) we don\u0027t ever mutate instances of SR once we create them\n\nThat\u0027s not true: even discounting the multitude of places where we\u0027re updating timestamps (since those don\u0027t impact equality), [merging donors into an acceptor](https://github.com/openstack/swift/blob/2.35.0/swift/container/sharder.py#L435) changes the acceptor\u0027s bounds (which *does* [impact equality](https://github.com/openstack/swift/blob/2.35.0/swift/common/utils/__init__.py#L3493)).\n\n\u003e b) we really only assert equivalency of different instances of SR in tests\n\nSo why\u0027d we implement it *at all*? You like to say YAGNI -- should we rip it out?\n\n\u003e I *think* this is changing the behavior? Is that on purpose?\n\nYep -- and since it\u0027s all in-process anyway, the exact implementation doesn\u0027t matter as long as the contract of `__hash__` holds. My question is, why were we writing our own `__hash__` function *at all*? I\u0027m not convinced we know what we\u0027re doing w.r.t `__hash__`, but at least [every](https://github.com/openstack/swift/blob/2.35.0/swift/common/manager.py#L508-L509) [other](https://github.com/openstack/swift/blob/2.35.0/swift/common/storage_policy.py#L210-L211) [place](https://github.com/openstack/swift/blob/2.35.0/swift/common/utils/timestamp.py#L249-L250) we define one, we have it based on `hash(self.something)`.","commit_id":"c177e7dcff31671ea2bd4de1da206bb286296568"}]}