Copilot commented on code in PR #16778:
URL: https://github.com/apache/pinot/pull/16778#discussion_r2332237713
##########
pinot-common/src/main/java/org/apache/pinot/common/config/DefaultClusterConfigChangeHandler.java:
##########
@@ -53,13 +56,15 @@ public void onClusterConfigChange(ClusterConfig
clusterConfig, NotificationConte
}
private synchronized void process(Map<String, String> properties) {
- Set<String> changedProperties = getChangedProperties(_properties,
properties);
- _properties = properties;
- _clusterConfigChangeListeners.forEach(l -> l.onChange(changedProperties,
_properties));
+ Set<String> changedProperties =
ImmutableSet.copyOf(getChangedProperties(_properties, properties));
+ _properties = ImmutableMap.copyOf(properties);
Review Comment:
The `ImmutableMap.copyOf(properties)` call creates a defensive copy on every
configuration change. If the `properties` parameter is already immutable or if
performance is critical, consider checking if copying is necessary or document
the performance implications of this defensive copying.
```suggestion
if (properties instanceof ImmutableMap) {
// Avoid unnecessary copy if already immutable
_properties = (ImmutableMap<String, String>) properties;
} else {
_properties = ImmutableMap.copyOf(properties);
}
```
--
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]
