Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2280080864
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -176,7 +182,7 @@ public Map<String, String> getMetadata() {
@Override
public SortedMap<String, String> getSortedMetadata() {
- return new TreeMap<>(getMetadata());
+ return metadata;
Review Comment:
Returning the internal metadata field directly breaks encapsulation and
allows external modification of the internal state. Consider returning a copy
or an unmodifiable view: Collections.unmodifiableSortedMap(metadata).
```suggestion
return Collections.unmodifiableSortedMap(metadata);
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +315,61 @@ 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());
- for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
- if
(entry.getKey().equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)) {
+ if (!Objects.equals(getServiceName(), that.getServiceName())
+ || !Objects.equals(getHost(), that.getHost())
+ || !Objects.equals(getPort(), that.getPort())) {
+ return false;
+ }
+
+ // compare map entry values by iterating over two sorted metadata maps
in a single pass.
+ Iterator<Map.Entry<String, String>> iterThis =
+ this.getSortedMetadata().entrySet().iterator();
+ Iterator<Map.Entry<String, String>> iterThat =
+ that.getSortedMetadata().entrySet().iterator();
+
+ boolean hasNext = iterThis.hasNext();
+ if (hasNext != iterThat.hasNext()) {
+ // keep consistency with hashCode, as any extra or less key would
result in a different hashCode.
+ return false;
+ }
+
+ while (hasNext) {
+ Map.Entry<String, String> entryThis = iterThis.next();
+ Map.Entry<String, String> entryThat = iterThat.next();
+
+ String key = entryThis.getKey();
+ if (!key.equals(entryThat.getKey())) {
+ // keep consistency with hashCode, as any extra or less key
would result in a different hashCode.
+ return false;
+ }
+
+ if (key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) ||
key.equals(TIMESTAMP_KEY)) {
+ // skip entry value comparison.
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(entryThis.getValue(), entryThat.getValue())) {
+ return false;
+ }
+
+ hasNext = iterThis.hasNext();
+ if (hasNext != iterThat.hasNext()) {
+ // keep consistency with hashCode, as any extra or less key
would result in a different hashCode.
+ return false;
}
}
- return equals;
+ return true;
Review Comment:
[nitpick] The iterator-based comparison is unnecessarily complex for this
use case. Since both maps are now TreeMaps with the same ordering, you could
simply compare the filtered maps directly using Objects.equals() after removing
the excluded keys, which would be more readable and maintainable.
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -57,7 +60,10 @@ public class DefaultServiceInstance implements
ServiceInstance {
private boolean healthy = true;
- private Map<String, String> metadata = new HashMap<>();
+ /**
+ * use SortedMap to ensure the hashCode of metadata will be calculated by
the same order
+ */
+ private SortedMap<String, String> metadata = new TreeMap<>();
Review Comment:
Changing the field type from Map to SortedMap is a breaking API change. This
could cause compilation errors for code that directly accesses this field or
relies on the specific Map implementation. Consider keeping the field as
Map<String, String> while internally using TreeMap implementation.
```suggestion
private Map<String, String> metadata = new TreeMap<>();
```
--
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]
