Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2278116480
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,21 +308,43 @@ 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<>();
Review Comment:
Consider initializing the HashSet with an estimated capacity based on
metadata size to avoid rehashing during insertion.
```suggestion
Set<String> checkedKeys = new HashSet<>(this.getMetadata().size());
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,21 +308,43 @@ 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<>();
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()));
+ if (null != that.getMetadata().get(key)) {
+ return false;
+ }
} else {
- equals = equals &&
entry.getValue().equals(that.getMetadata().get(entry.getKey()));
+ if (!entry.getValue().equals(that.getMetadata().get(key))) {
+ return false;
+ }
+ }
+ }
+
+ for (Map.Entry<String, String> entry : that.getMetadata().entrySet()) {
+ String otherKey = entry.getKey();
+ if (checkedKeys.contains(otherKey)
+ ||
otherKey.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)
+ || otherKey.equals(TIMESTAMP_KEY)) {
+ continue;
+ }
+ if (null != that.getMetadata().get(otherKey)) {
Review Comment:
This condition checks if the value is not null, but should check if the
value is null. When a key exists in 'that' but not in 'this' (since it wasn't
in checkedKeys), we should return false only if 'that' has a non-null value for
this key while 'this' doesn't have the key at all.
```suggestion
if (entry.getValue() != null) {
```
--
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]
