Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2280335544
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +310,60 @@ 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)) {
- continue;
+ if (!Objects.equals(serviceName, that.serviceName)
+ || !Objects.equals(host, that.host)
+ || !Objects.equals(port, that.port)) {
+ return false;
+ }
+
+ // compare metadata.
+ return compareMetadata(that.metadata);
+ }
+
Review Comment:
The `compareMetadata` method lacks documentation explaining its purpose,
behavior with excluded keys, and the significance of the hashCode consistency
comments throughout the method.
```suggestion
/**
* Compares this instance's metadata with another {@code
SortedMap<String, String>} for equality.
* <p>
* The comparison is performed in key order. For each key:
* <ul>
* <li>If the key is {@link
ServiceInstanceMetadataUtils#EXPORTED_SERVICES_REVISION_PROPERTY_NAME} or
{@link CommonConstants#TIMESTAMP_KEY},
* only the presence and order of the key are checked; their
values are ignored.</li>
* <li>For all other keys, both the key and its value must match.</li>
* </ul>
* <p>
* This method is designed to ensure consistency with the {@code
hashCode} implementation,
* as indicated by the inline comments. Any difference in key presence,
order, or (for non-excluded keys) value
* will result in {@code false}.
*
* @param thatMetadata the metadata map to compare with
* @return {@code true} if the metadata maps are considered equal
according to the above rules; {@code false} otherwise
*/
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +310,60 @@ 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)) {
- continue;
+ if (!Objects.equals(serviceName, that.serviceName)
+ || !Objects.equals(host, that.host)
+ || !Objects.equals(port, that.port)) {
+ return false;
+ }
+
+ // compare metadata.
+ return compareMetadata(that.metadata);
+ }
+
+ private boolean compareMetadata(SortedMap<String, String> thatMetadata) {
+ Iterator<Map.Entry<String, String>> thisIter =
this.metadata.entrySet().iterator();
+ Iterator<Map.Entry<String, String>> thatIter =
thatMetadata.entrySet().iterator();
+
+ boolean hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less key
between two metadata.
+ return false;
+ }
+
+ while (hasNext) {
+ Map.Entry<String, String> thisEntry = thisIter.next();
+ Map.Entry<String, String> thatEntry = thatIter.next();
+
+ String key = thisEntry.getKey();
+ if (!key.equals(thatEntry.getKey())) {
+ // hashCode will be different if there are an extra or less
key between two metadata.
+ return false;
}
- 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 (!key.equals(EXPORTED_SERVICES_REVISION_PROPERTY_NAME) &&
!key.equals(TIMESTAMP_KEY)) {
+ if (!Objects.equals(thisEntry.getValue(),
thatEntry.getValue())) {
+ return false;
+ }
+ }
+
+ hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less
key between two metadata.
+ return false;
}
}
- return equals;
+ return true;
}
@Override
public int hashCode() {
int result = Objects.hash(getServiceName(), getHost(), getPort());
+
+ // use SortedMap as metadata type to ensure the hashCode calculation
is consistent with equals method.
for (Map.Entry<String, String> entry : this.getMetadata().entrySet()) {
Review Comment:
The hashCode method calls `this.getMetadata().entrySet()` which creates a
new TreeMap via `getSortedMetadata()`, but could directly iterate over
`this.metadata.entrySet()` for better performance and consistency with the
equals method.
```suggestion
for (Map.Entry<String, String> entry : this.metadata.entrySet()) {
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -305,28 +310,60 @@ 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)) {
- continue;
+ if (!Objects.equals(serviceName, that.serviceName)
+ || !Objects.equals(host, that.host)
+ || !Objects.equals(port, that.port)) {
+ return false;
+ }
+
+ // compare metadata.
+ return compareMetadata(that.metadata);
+ }
+
+ private boolean compareMetadata(SortedMap<String, String> thatMetadata) {
+ Iterator<Map.Entry<String, String>> thisIter =
this.metadata.entrySet().iterator();
+ Iterator<Map.Entry<String, String>> thatIter =
thatMetadata.entrySet().iterator();
+
+ boolean hasNext = thisIter.hasNext();
+ if (hasNext != thatIter.hasNext()) {
+ // hashCode will be different if there are an extra or less key
between two metadata.
Review Comment:
The condition `hasNext != thatIter.hasNext()` is checked in multiple places
with identical comments. This logic could be extracted into a helper method to
reduce duplication and improve maintainability.
```suggestion
if (!iteratorsInSync(hasNext, thatIter)) {
```
--
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: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
