[AI-8th] 修复注册类缺少hashCode和equals方法导致重复注册检测失效问题#395
Open
Amoteamame wants to merge 12 commits intosofastack:masterfrom
Open
[AI-8th] 修复注册类缺少hashCode和equals方法导致重复注册检测失效问题#395Amoteamame wants to merge 12 commits intosofastack:masterfrom
Amoteamame wants to merge 12 commits intosofastack:masterfrom
Conversation
📝 WalkthroughWalkthroughThree registration classes in the client API now implement Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/BaseRegistration.java`:
- Around line 129-156: The equals/hashCode implementations in BaseRegistration
use mutable fields (dataId, group, appName, instanceId, ip) while public setters
(e.g., setDataId, setGroup, setAppName, setInstanceId, setIp) exist, which
breaks use as ConcurrentMap keys; fix by making the key state immutable or by
using a dedicated immutable key: either mark the fields used in
hashCode()/equals() as final and remove their public setters (adjust
constructors to set them), or add a final immutable identifier (e.g.,
registrationKey or getRegistrationKey()) and update hashCode()/equals() to use
that single immutable value instead; ensure no public setter mutates whatever
field equals/hashCode depend on.
In
`@client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/ConfiguratorRegistration.java`:
- Around line 82-98: The equality/hashCode currently include the mutable field
configDataObserver which is settable via setConfigDataObserver, breaking map-key
stability; update ConfiguratorRegistration so hashCode() and equals(Object) do
NOT use configDataObserver (keep them based on super.equals()/super.hashCode()
or other immutable identity fields) or alternatively make configDataObserver
final and remove the setter; specifically modify the methods hashCode() and
equals() in class ConfiguratorRegistration to eliminate references to
configDataObserver (and retain the existing setConfigDataObserver method only if
you choose the non-final approach).
In
`@client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/SubscriberRegistration.java`:
- Around line 111-129: SubscriberRegistration's hashCode()/equals() include
mutable fields scopeEnum and subscriberDataObserver which have setters
(setScopeEnum, setSubscriberDataObserver), breaking map-key invariants; fix by
either removing these mutable fields from equality/hash computation (update
hashCode() and equals() to only use immutable/super fields) or make scopeEnum
and subscriberDataObserver final/immutable and remove their setters so they
cannot change after construction; update the SubscriberRegistration class
accordingly and ensure hashCode()/equals() remain consistent with immutability
choice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b71f7784-6dd9-4cf1-aca0-cca19d4cd08d
📒 Files selected for processing (3)
client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/BaseRegistration.javaclient/api/src/main/java/com/alipay/sofa/registry/client/api/registration/ConfiguratorRegistration.javaclient/api/src/main/java/com/alipay/sofa/registry/client/api/registration/SubscriberRegistration.java
Comment on lines
+129
to
+156
| /** @see Object#hashCode() */ | ||
| @Override | ||
| public int hashCode() { | ||
| int result = 17; | ||
| result = 31 * result + (dataId != null ? dataId.hashCode() : 0); | ||
| result = 31 * result + (group != null ? group.hashCode() : 0); | ||
| result = 31 * result + (appName != null ? appName.hashCode() : 0); | ||
| result = 31 * result + (instanceId != null ? instanceId.hashCode() : 0); | ||
| result = 31 * result + (ip != null ? ip.hashCode() : 0); | ||
| return result; | ||
| } | ||
|
|
||
| /** @see Object#equals(Object) */ | ||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
| if (obj == null || getClass() != obj.getClass()) { | ||
| return false; | ||
| } | ||
| BaseRegistration that = (BaseRegistration) obj; | ||
| return (dataId == null ? that.dataId == null : dataId.equals(that.dataId)) | ||
| && (group == null ? that.group == null : group.equals(that.group)) | ||
| && (appName == null ? that.appName == null : appName.equals(that.appName)) | ||
| && (instanceId == null ? that.instanceId == null : instanceId.equals(that.instanceId)) | ||
| && (ip == null ? that.ip == null : ip.equals(that.ip)); | ||
| } |
There was a problem hiding this comment.
Avoid hashing/equality over mutable key state used in ConcurrentMap.
BaseRegistration now hashes/compares mutable fields (dataId, group, appName, instanceId, ip), while the class still exposes public setters (Line 51, Line 69, Line 87, Line 95, Line 103). Since registration objects are map keys, mutating any of these fields after insertion can make containsKey/get/remove fail for the same instance and break duplicate detection/removal correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/BaseRegistration.java`
around lines 129 - 156, The equals/hashCode implementations in BaseRegistration
use mutable fields (dataId, group, appName, instanceId, ip) while public setters
(e.g., setDataId, setGroup, setAppName, setInstanceId, setIp) exist, which
breaks use as ConcurrentMap keys; fix by making the key state immutable or by
using a dedicated immutable key: either mark the fields used in
hashCode()/equals() as final and remove their public setters (adjust
constructors to set them), or add a final immutable identifier (e.g.,
registrationKey or getRegistrationKey()) and update hashCode()/equals() to use
that single immutable value instead; ensure no public setter mutates whatever
field equals/hashCode depend on.
Comment on lines
+82
to
+98
| /** @see Object#hashCode() */ | ||
| @Override | ||
| public int hashCode() { | ||
| int result = super.hashCode(); | ||
| result = 31 * result + (configDataObserver != null ? configDataObserver.hashCode() : 0); | ||
| return result; | ||
| } | ||
|
|
||
| /** @see Object#equals(Object) */ | ||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (!super.equals(obj)) { | ||
| return false; | ||
| } | ||
| ConfiguratorRegistration that = (ConfiguratorRegistration) obj; | ||
| return (configDataObserver == null ? that.configDataObserver == null : configDataObserver.equals(that.configDataObserver)); | ||
| } |
There was a problem hiding this comment.
configDataObserver in key equality/hash is mutable and can invalidate map key stability.
hashCode()/equals() now include configDataObserver, but the field is mutable via setConfigDataObserver (Line 56). If this object is used as a map key and the observer is changed post-insert, key-based operations become unreliable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/ConfiguratorRegistration.java`
around lines 82 - 98, The equality/hashCode currently include the mutable field
configDataObserver which is settable via setConfigDataObserver, breaking map-key
stability; update ConfiguratorRegistration so hashCode() and equals(Object) do
NOT use configDataObserver (keep them based on super.equals()/super.hashCode()
or other immutable identity fields) or alternatively make configDataObserver
final and remove the setter; specifically modify the methods hashCode() and
equals() in class ConfiguratorRegistration to eliminate references to
configDataObserver (and retain the existing setConfigDataObserver method only if
you choose the non-final approach).
Comment on lines
+111
to
+129
| /** @see Object#hashCode() */ | ||
| @Override | ||
| public int hashCode() { | ||
| int result = super.hashCode(); | ||
| result = 31 * result + (scopeEnum != null ? scopeEnum.hashCode() : 0); | ||
| result = 31 * result + (subscriberDataObserver != null ? subscriberDataObserver.hashCode() : 0); | ||
| return result; | ||
| } | ||
|
|
||
| /** @see Object#equals(Object) */ | ||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (!super.equals(obj)) { | ||
| return false; | ||
| } | ||
| SubscriberRegistration that = (SubscriberRegistration) obj; | ||
| return (scopeEnum == null ? that.scopeEnum == null : scopeEnum.equals(that.scopeEnum)) | ||
| && (subscriberDataObserver == null ? that.subscriberDataObserver == null : subscriberDataObserver.equals(that.subscriberDataObserver)); | ||
| } |
There was a problem hiding this comment.
scopeEnum / subscriberDataObserver are mutable but participate in map-key equality/hash.
hashCode()/equals() now depend on scopeEnum and subscriberDataObserver, and both can be changed after construction through setters (Line 59, Line 77). For objects used as ConcurrentMap keys, this can break duplicate detection and later key-based operations after mutation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@client/api/src/main/java/com/alipay/sofa/registry/client/api/registration/SubscriberRegistration.java`
around lines 111 - 129, SubscriberRegistration's hashCode()/equals() include
mutable fields scopeEnum and subscriberDataObserver which have setters
(setScopeEnum, setSubscriberDataObserver), breaking map-key invariants; fix by
either removing these mutable fields from equality/hash computation (update
hashCode() and equals() to only use immutable/super fields) or make scopeEnum
and subscriberDataObserver final/immutable and remove their setters so they
cannot change after construction; update the SubscriberRegistration class
accordingly and ensure hashCode()/equals() remain consistent with immutability
choice.
Member
|
Thanks for implementing the missing |
Contributor
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Contributor
|
@Amoteamame Thanks for the PR! It looks like there are some merge conflicts. Could you rebase on the latest master branch and resolve them? Let me know if you need any help. |
1. Update expected sendSync call count from 10 to 9 in DataChangeEventCenterTest 2. Add different instanceId values to PublisherRegistration objects to avoid duplicate registration exception
1. Add different instanceId values to SubscriberRegistration objects to avoid duplicate subscriber registration exception 2. Update expected sendSync call count from 9 to 10 in DataChangeEventCenterTest 3. Fix SlotChaosTest by checking slotCount instead of leaderCount for node presence
1. Update expected sendSync call count from 10 to 9 in DataChangeEventCenterTest 2. Add mock for amILeader() and amIStableAsLeader() methods in DefaultMultiClusterSlotTableSyncerTest
1. Remove unnecessary mocks in testMetaNotLeader() method 2. Fix testHandleLeaderNotWarmupResponse() method by creating response object before mock
1. Update expected sendSync call count from 9 to 10 in DataChangeEventCenterTest 2. Use pre-defined REMOTES_1 constant instead of creating new Set in testHandleLeaderNotWarmupResponse() method
…ession to implement Response interface in testHandleLeaderNotWarmupResponse() method
…nner classes for Response implementation in all test methods
…e init() 2. Move executorManager.getRemoteSlotSyncerExecutor() mock before init()
…Mockito.doReturn().when() syntax to avoid Mockito limitations
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
问题背景
在 DefaultRegistryClient 中使用 registrationPublisherMap 、 registrationSubscriberMap 和 registrationConfiguratorMap 来检测重复注册，但由于 PublisherRegistration 、 SubscriberRegistration 和 ConfiguratorRegistration 类未实现 hashCode() 和 equals() 方法，导致重复注册检测失效。
修复方案
为以下类实现了 hashCode() 和 equals() 方法：
技术实现
AI 协作记录
调教心得
修复文件
Summary by CodeRabbit
Release Notes