ywxzm03 commented on PR #960: URL: https://github.com/apache/dubbo-go-pixiu/pull/960#issuecomment-4657602391
> Thanks for the optimization, and for the thorough tests and benchmarks. I checked the reuse predicate against what the consistent-hash rings actually consume. Before merging, I suggested making some minor modifications related to scope/maintainability, rather than correctness: **1. The reuse predicate compares fields the ring never reads — please scope it down** > > Both rings are built purely from `endpoint.GetHost()` (address:port): > > * `ringhash/ring_hash.go:55` — `h.Add(endpoint.GetHost())` > * `maglev/maglev_hash.go:36` — `hosts[i] = endpoint.GetHost()`, and `permutation.go` `NewLookUpTable` only ever receives `[]string` hosts. > > So `ID` (`cluster.go:630`), `Metadata` (`sameHashRelevantMetadata`, `cluster.go:653`) and `Domains` (`cluster.go:646`) don't affect the ring. Comparing them only makes reuse more conservative — extra per-refresh work on what's meant to be a fast path, for zero correctness benefit. Please collapse `sameEndpointForConsistentHash` to compare exactly what the ring uses (`GetHost()` plus the config already covered by `sameConsistentHashConfig`) and drop the ID/metadata/domain helpers. > > (Aside: the domain check at `cluster.go:646` only compares `Domains[0]`, not the whole slice — so it isn't even a complete identity check, which is one more reason to remove rather than extend it.) > > **2. `sameHashRelevantMetadata` is a misleading name** > > Metadata isn't hash-relevant; the ring never reads it. Even if any comparison were kept, this name plants a wrong mental model for future maintainers. It should go away with #1. > > **3. The metadata test encodes a contract that doesn't exist** > > `TestClusterEndpointSnapshotDoesNotReuseConsistentHashWhenMetadataChanges` (`cluster_test.go:374`) asserts a metadata-only change must rebuild. That pins an implementation detail rather than a routing requirement, and it will need to change with #1 — once reuse keys on host only, a metadata-only change with an unchanged host set can safely reuse. Please update/remove it alongside the logic. > > **Non-blocking nit:** `registerCountingConsistentHash` mutates the global `model.ConsistentHashInitMap` (`cluster_test.go:712`) and restores via `t.Cleanup`. Fine today since these tests run serially, but it'll race if any of them ever gets `t.Parallel()`. Worth a guard or a comment. fixed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
