Copilot commented on code in PR #15633:
URL: https://github.com/apache/dubbo/pull/15633#discussion_r2280283284
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -59,6 +60,8 @@ public class DefaultServiceInstance implements
ServiceInstance {
private Map<String, String> metadata = new HashMap<>();
Review Comment:
The `identicalMetadata` field lacks documentation explaining its purpose and
relationship to the main `metadata` field. Consider adding a comment explaining
that this field stores metadata excluding timestamp and revision keys for
equality comparison.
```suggestion
/**
* Stores a copy of the metadata map excluding timestamp and revision
keys.
* Used for equality comparison to ignore changes in those specific keys.
* This field is related to {@link #metadata} but omits keys such as
TIMESTAMP_KEY and EXPORTED_SERVICES_REVISION_PROPERTY_NAME.
*/
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -91,6 +94,7 @@ public DefaultServiceInstance(DefaultServiceInstance other) {
this.applicationModel = other.applicationModel;
this.extendParams = other.extendParams != null ? new
HashMap<>(other.extendParams) : other.extendParams;
this.endpoints = other.endpoints != null ? new
ArrayList<>(other.endpoints) : other.endpoints;
+ setIdenticalMetadata(this.metadata);
Review Comment:
The `setIdenticalMetadata` call should use `other.metadata` instead of
`this.metadata` since `this.metadata` hasn't been set yet in the copy
constructor. This could result in an empty `identicalMetadata` if the original
metadata is null.
```suggestion
setIdenticalMetadata(other.metadata);
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -254,14 +260,17 @@ public DefaultServiceInstance copyFrom(int port) {
@Override
public Map<String, String> getAllParams() {
+ // return copy to avoid external modification of the internal state.
+ Map<String, String> allParams;
if (extendParams == null) {
- return metadata;
+ allParams = new HashMap<>((int) (metadata.size() / 0.75f + 1));
Review Comment:
The magic number `0.75f` represents the HashMap default load factor but
should be extracted as a named constant for better maintainability and clarity.
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/DefaultServiceInstance.java:
##########
@@ -350,6 +351,13 @@ public String toFullString() {
+ metadata + '}';
}
+ private void setIdenticalMetadata(Map<String, String> metadata) {
+ TreeMap<String, String> identicalMetadata = new TreeMap<>(metadata);
Review Comment:
The method doesn't handle null metadata input, which could cause a
NullPointerException when creating the TreeMap. Add a null check to handle this
case gracefully.
```suggestion
TreeMap<String, String> identicalMetadata = new TreeMap<>(metadata
== null ? Collections.emptyMap() : metadata);
```
--
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]
