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/22 00:34:47 UTC
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9808: [Draft] [multistage] Add Multi Stage Routing Strategy
Jackie-Jiang commented on code in PR #9808:
URL: https://github.com/apache/pinot/pull/9808#discussion_r1028636742
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -295,6 +289,6 @@ public SelectionResult select(BrokerRequest brokerRequest, List<String> segments
* <p>NOTE: {@code segmentToEnabledInstancesMap} might contain {@code null} values (segment with no enabled
* ONLINE/CONSUMING instances). If enabled instances are not {@code null}, they are sorted in alphabetical order.
*/
- abstract Map<String, String> select(List<String> segments, int requestId,
+ abstract Map<String, String> select(List<String> segments, long requestId,
Review Comment:
Let's not change this signature. It expects a non-negative int
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorFactory.java:
##########
@@ -38,7 +40,7 @@ private InstanceSelectorFactory() {
public static final String LEGACY_REPLICA_GROUP_REALTIME_ROUTING = "PartitionAwareRealtime";
public static InstanceSelector getInstanceSelector(TableConfig tableConfig, BrokerMetrics brokerMetrics,
- @Nullable AdaptiveServerSelector adaptiveServerSelector) {
+ ZkHelixPropertyStore<ZNRecord> propertyStore, @Nullable AdaptiveServerSelector adaptiveServerSelector) {
Review Comment:
(nit) Put `propertyStore` before `brokerMetrics`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotBrokerDebug.java:
##########
@@ -102,15 +102,15 @@ public Map<String, Map<ServerInstance, List<String>>> getRoutingTable(
if (tableType != TableType.REALTIME) {
String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(tableName);
RoutingTable routingTable = _routingManager.getRoutingTable(
- CalciteSqlCompiler.compileToBrokerRequest("SELECT * FROM " + offlineTableName));
+ CalciteSqlCompiler.compileToBrokerRequest("SELECT * FROM " + offlineTableName), 0);
Review Comment:
IIRC, presto connector relies on this to return the routing table. Let's add a `requestIdGenerator` in this class to avoid always using the same request id.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java:
##########
@@ -267,8 +262,7 @@ private List<String> calculateEnabledInstancesForSegment(String segment, List<St
}
@Override
- public SelectionResult select(BrokerRequest brokerRequest, List<String> segments) {
- int requestId = (int) (_requestId.getAndIncrement() % MAX_REQUEST_ID);
+ public SelectionResult select(BrokerRequest brokerRequest, List<String> segments, long requestId) {
Review Comment:
Keep the `MAX_REQUEST_ID` and pass `(int) (requestId % MAX_REQUEST_ID);`
--
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