Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2278781552
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +308,45 @@ public boolean equals(Object o) {
return false;
}
DefaultServiceInstance that = (DefaultServiceInstance) o;
- boolean equals = Objects.equals(getServiceName(),
that.getServiceName())
- && Objects.equals(getHost(), that.getHost())
- && Objects.equals(getPort(), that.getPort());
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
+ }
+
+ Set<String> checkedKeys = new HashSet<>(this.getMetadata().size());
for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
+ String key = entry.getKey();
+ checkedKeys.add(key);
+ if (key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) ||
key.equals(TIMESTAMP_KEY)) {
continue;
}
- if (entry.getValue() == null) {
- equals = equals && (entry.getValue() ==
that.getMetadata().get(entry.getKey()));
- } else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
+ if (!Objects.equals(entry.getValue(),
that.getMetadata().get(key))) {
+ return false;
+ }
+ }
+
+ for (Map.Entry<String, String> thatEntry :
that.getMetadata().entrySet()) {
+ String thatKey = thatEntry.getKey();
+ if (checkedKeys.contains(thatKey)
+ || thatKey.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)
+ || thatKey.equals(TIMESTAMP_KEY)) {
+ continue;
}
+ // return false if this instance doesn't have the key,
+ // because the hashCode will be different no matter that entry
value is null or not.
Review Comment:
[nitpick] The comment on line 336-337 explains the logic, but the immediate
return false without checking the actual value could be confusing. Consider
adding a more descriptive comment explaining that any extra key in 'that'
instance (not present in 'this') makes them unequal, regardless of the value.
```suggestion
// If 'that' instance contains a key not present in 'this'
instance (excluding special keys),
// consider them unequal, regardless of the value. This ensures
consistency with hashCode,
// as any extra key in 'that' would result in a different
hashCode.
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +308,45 @@ public boolean equals(Object o) {
return false;
}
DefaultServiceInstance that = (DefaultServiceInstance) o;
- boolean equals = Objects.equals(getServiceName(),
that.getServiceName())
- && Objects.equals(getHost(), that.getHost())
- && Objects.equals(getPort(), that.getPort());
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
+ }
+
+ Set<String> checkedKeys = new HashSet<>(this.getMetadata().size());
Review Comment:
The HashSet is created with the size of this.getMetadata() but will only
contain keys that are not skipped (excluding
EXPORTED_SERVICES_REVISION_PROPERTY_NAME and TIMESTAMP_KEY). Consider using a
smaller initial capacity or the default constructor to avoid over-allocation.
```suggestion
Set<String> checkedKeys = new HashSet<>();
```
--
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]
