You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/15 08:37:36 UTC

[GitHub] [pinot] vvivekiyer opened a new pull request, #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

vvivekiyer opened a new pull request, #9803:
URL: https://github.com/apache/pinot/pull/9803

   When AdaptiveServerSelector is used with `replicaGroupInstanceSelector`, it fetches the ranking of all available servers. If a broker is routing to multiple server tenants, we will be unnecessarily fetching the ranking for a lot more servers than required. Note that fetching server rankings also involves waiting for a read lock to fetch the stats. 
   
   In this PR, we first determine the candidate set of servers to which a query could be potentially routed. The ranking is performed only for servers in this candidate set. 


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9803:
URL: https://github.com/apache/pinot/pull/9803#discussion_r1034185618


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -156,4 +159,20 @@ private void selectServersUsingAdaptiverServerSelector(List<String> segments, in
       segmentToSelectedInstanceMap.put(segment, selectedInstance);
     }
   }
+
+  private List<String> fetchCandidateServersForQuery(List<String> segments,
+      Map<String, List<String>> segmentToEnabledInstancesMap) {
+    List<String> serversList = new ArrayList<>();
+
+    Set<String> tempServerSet = new HashSet<>();
+    for (String segment : segments) {
+      List<String> enabledInstances = segmentToEnabledInstancesMap.get(segment);
+      if (enabledInstances != null) {
+        tempServerSet.addAll(enabledInstances);
+      }
+    }
+
+    serversList.addAll(tempServerSet);
+    return serversList;
+  }

Review Comment:
   Got it. Thanks for clarifying 



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9803:
URL: https://github.com/apache/pinot/pull/9803#discussion_r1023128025


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/adaptiveserverselector/LatencySelector.java:
##########
@@ -79,4 +81,30 @@ public List<Pair<String, Double>> fetchAllServerRankingsWithScores() {
 
     return pairList;
   }
+
+  @Override

Review Comment:
   (nit) may want to add a javadoc to highlight what differentiates this and the existing API and the rationale (like discussed internally) behind adding it



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9803:
URL: https://github.com/apache/pinot/pull/9803#discussion_r1023137053


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -156,4 +159,20 @@ private void selectServersUsingAdaptiverServerSelector(List<String> segments, in
       segmentToSelectedInstanceMap.put(segment, selectedInstance);
     }
   }
+
+  private List<String> fetchCandidateServersForQuery(List<String> segments,
+      Map<String, List<String>> segmentToEnabledInstancesMap) {
+    List<String> serversList = new ArrayList<>();
+
+    Set<String> tempServerSet = new HashSet<>();
+    for (String segment : segments) {
+      List<String> enabledInstances = segmentToEnabledInstancesMap.get(segment);
+      if (enabledInstances != null) {
+        tempServerSet.addAll(enabledInstances);
+      }
+    }
+
+    serversList.addAll(tempServerSet);
+    return serversList;
+  }

Review Comment:
   hmm I am confused why should this code need to be implemented. 
   
   The logic used by replica group instance selector to pick candidate instances first is not getting changes by adaptive selection as the latter runs on top of it so shouldn't this logic already exist in the replica group instance selector ?



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia merged pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #9803:
URL: https://github.com/apache/pinot/pull/9803


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] codecov-commenter commented on pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9803:
URL: https://github.com/apache/pinot/pull/9803#issuecomment-1315014746

   # [Codecov](https://codecov.io/gh/apache/pinot/pull/9803?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#9803](https://codecov.io/gh/apache/pinot/pull/9803?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b7e27a9) into [master](https://codecov.io/gh/apache/pinot/commit/58c1dc9b7d8e28ac7207fa7565b1ed76f7a0c2ad?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (58c1dc9) will **decrease** coverage by `47.31%`.
   > The diff coverage is `69.49%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #9803       +/-   ##
   =============================================
   - Coverage     63.05%   15.73%   -47.32%     
   + Complexity     5235      175     -5060     
   =============================================
     Files          1942     1908       -34     
     Lines        104467   102617     -1850     
     Branches      15833    15608      -225     
   =============================================
   - Hits          65871    16149    -49722     
   - Misses        33707    85273    +51566     
   + Partials       4889     1195     -3694     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `15.73% <69.49%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9803?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...instanceselector/ReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1JlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `35.00% <0.00%> (+2.34%)` | :arrow_up: |
   | [...routing/adaptiveserverselector/HybridSelector.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9hZGFwdGl2ZXNlcnZlcnNlbGVjdG9yL0h5YnJpZFNlbGVjdG9yLmphdmE=) | `94.59% <85.71%> (+94.59%)` | :arrow_up: |
   | [...outing/adaptiveserverselector/LatencySelector.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9hZGFwdGl2ZXNlcnZlcnNlbGVjdG9yL0xhdGVuY3lTZWxlY3Rvci5qYXZh) | `94.59% <85.71%> (+94.59%)` | :arrow_up: |
   | [...adaptiveserverselector/NumInFlightReqSelector.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9hZGFwdGl2ZXNlcnZlcnNlbGVjdG9yL051bUluRmxpZ2h0UmVxU2VsZWN0b3IuamF2YQ==) | `95.23% <85.71%> (+95.23%)` | :arrow_up: |
   | [...query/runtime/operator/MailboxReceiveOperator.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94UmVjZWl2ZU9wZXJhdG9yLmphdmE=) | `80.32% <100.00%> (+1.38%)` | :arrow_up: |
   | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1677 more](https://codecov.io/gh/apache/pinot/pull/9803/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] vvivekiyer commented on a diff in pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
vvivekiyer commented on code in PR #9803:
URL: https://github.com/apache/pinot/pull/9803#discussion_r1023285220


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/adaptiveserverselector/LatencySelector.java:
##########
@@ -79,4 +81,30 @@ public List<Pair<String, Double>> fetchAllServerRankingsWithScores() {
 
     return pairList;
   }
+
+  @Override

Review Comment:
   The java doc is added in the interface file. Added more details to clarify the rationale.



##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -156,4 +159,20 @@ private void selectServersUsingAdaptiverServerSelector(List<String> segments, in
       segmentToSelectedInstanceMap.put(segment, selectedInstance);
     }
   }
+
+  private List<String> fetchCandidateServersForQuery(List<String> segments,
+      Map<String, List<String>> segmentToEnabledInstancesMap) {
+    List<String> serversList = new ArrayList<>();
+
+    Set<String> tempServerSet = new HashSet<>();
+    for (String segment : segments) {
+      List<String> enabledInstances = segmentToEnabledInstancesMap.get(segment);
+      if (enabledInstances != null) {
+        tempServerSet.addAll(enabledInstances);
+      }
+    }
+
+    serversList.addAll(tempServerSet);
+    return serversList;
+  }

Review Comment:
   It already exists. But they way the current code is written is, we do only one loop over segments. For each segment, we pick the server from the list of servers for the segment. 
   
   With AdaptiveServerSelection, we do an initial loop before to determine the list of servers for all segments that are touched by a query.  Then a ranking on them is performed. And then loop over the segments again and now determine which server to pick for a segment. 



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] jadami10 commented on a diff in pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
jadami10 commented on code in PR #9803:
URL: https://github.com/apache/pinot/pull/9803#discussion_r1036226603


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/adaptiveserverselector/HybridSelector.java:
##########
@@ -85,4 +87,30 @@ public List<Pair<String, Double>> fetchAllServerRankingsWithScores() {
 
     return pairList;
   }
+
+  @Override
+  public List<Pair<String, Double>> fetchServerRankingsWithScores(List<String> serverCandidates) {
+    List<Pair<String, Double>> pairList = new ArrayList<>();
+    if (serverCandidates.size() == 0) {
+      return pairList;
+    }
+
+    for (String server : serverCandidates) {
+      Double score = _serverRoutingStatsManager.fetchHybridScoreForServer(server);
+      if (score == null) {
+        score = -1.0;
+      }
+
+      pairList.add(new ImmutablePair<>(server, score));
+    }
+
+    // Let's shuffle the list before sorting. This helps with randomly choosing different servers if there is a tie.
+    Collections.shuffle(pairList);

Review Comment:
   nit: this feels unnecessarily expensive if there are a lot of servers. Can it be done by randomly returning 1 or -1 when val==0 in the `.sort` below?



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9803: Optimize AdaptiveServerSelection for replicaGroup based routing

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9803:
URL: https://github.com/apache/pinot/pull/9803#discussion_r1023137053


##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -156,4 +159,20 @@ private void selectServersUsingAdaptiverServerSelector(List<String> segments, in
       segmentToSelectedInstanceMap.put(segment, selectedInstance);
     }
   }
+
+  private List<String> fetchCandidateServersForQuery(List<String> segments,
+      Map<String, List<String>> segmentToEnabledInstancesMap) {
+    List<String> serversList = new ArrayList<>();
+
+    Set<String> tempServerSet = new HashSet<>();
+    for (String segment : segments) {
+      List<String> enabledInstances = segmentToEnabledInstancesMap.get(segment);
+      if (enabledInstances != null) {
+        tempServerSet.addAll(enabledInstances);
+      }
+    }
+
+    serversList.addAll(tempServerSet);
+    return serversList;
+  }

Review Comment:
   hmm I am confused why should this code need to be implemented. 
   
   The logic used by replica group instance selector to pick candidate instances first is not getting changed by adaptive selection as the latter runs on top of it so shouldn't this logic already exist in the replica group instance selector ?



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org