Copilot commented on code in PR #17605:
URL: https://github.com/apache/pinot/pull/17605#discussion_r2747953582
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -323,12 +327,24 @@ private ExternalView getExternalView(String
externalViewPath) {
ZNRecord znRecord = _zkDataAccessor.get(externalViewPath, stat,
AccessOption.PERSISTENT);
if (znRecord != null) {
znRecord.setVersion(stat.getVersion());
+ // Intern all the instance id and state to reduce memory footprint
+ internMapFields(znRecord);
+ // TODO: Avoid copying ZNRecord. This requires Helix change.
return new ExternalView(znRecord);
} else {
return null;
}
}
+ private void internMapFields(ZNRecord znRecord) {
+ for (Map.Entry<String, Map<String, String>> entry :
znRecord.getMapFields().entrySet()) {
+ Map<String, String> instanceStateMap = entry.getValue();
+ Map<String, String> internedInstanceStateMap = new TreeMap<>();
Review Comment:
Using TreeMap here adds unnecessary sorting overhead since the iteration
order doesn't appear to matter for routing purposes. Consider using HashMap
instead to avoid the O(log n) insertion cost of TreeMap. If preserving order is
important for debugging or consistency, document why TreeMap is necessary.
```suggestion
Map<String, String> internedInstanceStateMap = new
HashMap<>(instanceStateMap.size());
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -384,14 +400,15 @@ private void processInstanceConfigChangeInternal() {
for (ZNRecord instanceConfigZNRecord : instanceConfigZNRecords) {
// Put instance initialization logics into try-catch block to prevent
bad server configs affecting the entire
// cluster
- String instanceId = instanceConfigZNRecord.getId();
try {
if (isEnabledServer(instanceConfigZNRecord)) {
+ // Intern the instance id to reduce memory footprint
+ String instanceId = instanceConfigZNRecord.getId().intern();
Review Comment:
The instanceId variable is now declared inside the if block at line 406, but
the catch block at line 428 references instanceConfigZNRecord.getId() instead
of the instanceId variable. If an exception occurs within the if block after
line 406, the log message will not use the interned string, which is
inconsistent with the interning optimization. While this doesn't break
functionality, it means the exception handling doesn't benefit from the memory
optimization. Consider declaring instanceId before the if statement or ensuring
the catch block uses instanceId if it's in scope.
--
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]
