Jackie-Jiang commented on code in PR #19530:
URL: https://github.com/apache/pinot/pull/19530#discussion_r4009346398
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java:
##########
@@ -45,11 +47,13 @@ public class BalancedInstanceSelector extends
BaseInstanceSelector {
@Override
public InstanceMapping select(List<String> segments, int requestId,
SegmentStates segmentStates, Map<String, String> queryOptions) {
- Map<String, String> segmentToSelectedInstanceMap = new
HashMap<>(HashUtil.getHashMapCapacity(segments.size()));
+ // Allocate the flat map only when a required segment is selected. It
avoids one map node per segment without
+ // reserving large arrays for queries whose segments are all optional or
unavailable.
+ Map<String, String> segmentToSelectedInstanceMap = null;
Review Comment:
Is this laziness really necessary? It should be very rare for a table to
have no online segment. Same for the other selector
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -416,6 +416,8 @@ private void processInstanceConfigChangeInternal() {
String instanceId = instanceConfigZNRecord.getId();
try {
if (isEnabledServer(instanceConfigZNRecord)) {
+ // Match the interned instance IDs decoded from IS/EV map keys for
per-segment routing lookups.
+ instanceId = instanceId.intern();
Review Comment:
Per the documentation of guava `Interner`, seems it is recommended to have a
central interner for instances
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -1193,30 +1195,34 @@ public RoutingTable getRoutingTable(BrokerRequest
brokerRequest, String tableNam
private Map<ServerInstance, SegmentsToQuery>
getServerInstanceToSegmentsMap(String tableNameWithType,
InstanceSelector.SelectionResult selectionResult) {
Map<ServerInstance, SegmentsToQuery> merged = new HashMap<>();
- for (Map.Entry<String, String> entry :
selectionResult.getSegmentToInstanceMap().entrySet()) {
- ServerInstance serverInstance =
_enabledServerInstanceMap.get(entry.getValue());
+ // Flat selection maps can traverse their arrays directly without
allocating an entry object per segment.
+ selectionResult.getSegmentToInstanceMap().forEach((segment, instanceId) ->
{
+ ServerInstance serverInstance =
_enabledServerInstanceMap.get(instanceId);
if (serverInstance != null) {
- SegmentsToQuery segmentsToQuery =
- merged.computeIfAbsent(serverInstance, k -> new
SegmentsToQuery(new ArrayList<>(), new ArrayList<>()));
- segmentsToQuery.getSegments().add(entry.getKey());
+ SegmentsToQuery segmentsToQuery = merged.get(serverInstance);
+ if (segmentsToQuery == null) {
+ segmentsToQuery = new SegmentsToQuery(new ArrayList<>(), new
ArrayList<>());
+ merged.put(serverInstance, segmentsToQuery);
+ }
+ segmentsToQuery.getSegments().add(segment);
Review Comment:
Why changing this one? This will add one map lookup
--
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]
