Jackie-Jiang commented on code in PR #16104:
URL: https://github.com/apache/pinot/pull/16104#discussion_r2146088847
##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNativeV2.java:
##########
@@ -394,14 +395,14 @@ public Map<String, String> getTraceInfo() {
}
@Override
- public void setReplicaGroups(@NotNull Set<Integer> replicaGroups) {
- _replicaGroups = replicaGroups;
+ public void setPools(@NotNull Set<Integer> replicaGroups) {
Review Comment:
Please also check other places
```suggestion
public void setPools(@NotNull Set<Integer> pools) {
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -809,9 +809,9 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
_brokerMetrics.addTimedValue(BrokerTimer.QUERY_TOTAL_TIME_MS,
totalTimeMs, TimeUnit.MILLISECONDS);
}
- for (int group : brokerResponse.getReplicaGroups()) {
- _brokerMetrics.addMeteredValue(BrokerMeter.REPLICA_QUERIES, 1,
-
BrokerMetrics.getTagForPreferredGroup(sqlNodeAndOptions.getOptions()),
String.valueOf(group));
+ for (int group : brokerResponse.getPools()) {
Review Comment:
(minor)
```suggestion
for (int pool : brokerResponse.getPools()) {
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -194,13 +194,17 @@ public static Integer
getNumReplicaGroupsToQuery(Map<String, String> queryOption
}
public static List<Integer> getOrderedPreferredReplicas(Map<String, String>
queryOptions) {
- String orderedPreferredReplicas =
queryOptions.get(QueryOptionKey.ORDERED_PREFERRED_REPLICAS);
- if (orderedPreferredReplicas == null) {
+ String orderedPreferredPools =
queryOptions.get(QueryOptionKey.ORDERED_PREFERRED_POOLS);
+ if (StringUtils.isEmpty(orderedPreferredPools)) {
+ // backward compatibility
+ orderedPreferredPools =
queryOptions.get(QueryOptionKey.ORDERED_PREFERRED_REPLICAS);
+ }
+ if (StringUtils.isEmpty(orderedPreferredPools)) {
return Collections.emptyList();
}
// cannot use comma as the delimiter of replica group list
Review Comment:
Also update the comment?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -524,7 +527,9 @@ public static class QueryOptionKey {
public static final String NUM_REPLICA_GROUPS_TO_QUERY =
"numReplicaGroupsToQuery";
+ @Deprecated
public static final String ORDERED_PREFERRED_REPLICAS =
"orderedPreferredReplicas";
Review Comment:
+1. Can we just modify the name and not worry about backward compatibility?
##########
pinot-common/src/test/java/org/apache/pinot/common/metrics/BrokerMetricsTest.java:
##########
@@ -30,27 +30,33 @@
public class BrokerMetricsTest {
@Test
- public void testGetTagForPreferredGroup() {
+ public void testGetTagForPreferredPool() {
// Test case 1: queryOption is null
- assertEquals(BrokerMetrics.getTagForPreferredGroup(null),
"preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption is null");
+ assertEquals(BrokerMetrics.getTagForPreferredPool(null),
"preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption is null");
// Test case 2: queryOption is empty
Map<String, String> emptyQueryOption = new HashMap<>();
- assertEquals(BrokerMetrics.getTagForPreferredGroup(emptyQueryOption),
"preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption is empty");
+ assertEquals(BrokerMetrics.getTagForPreferredPool(emptyQueryOption),
"preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption is empty");
- // Test case 3: queryOption does not contain ORDERED_PREFERRED_REPLICAS
- Map<String, String> queryOptionWithoutPreferredGroup = new HashMap<>();
- queryOptionWithoutPreferredGroup.put("someOtherOption", "value");
-
assertEquals(BrokerMetrics.getTagForPreferredGroup(queryOptionWithoutPreferredGroup),
- "preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption does not
contain ORDERED_PREFERRED_REPLICAS");
+ // Test case 3: queryOption does not contain ORDERED_PREFERRED_POOLS
+ Map<String, String> queryOptionWithoutPreferredPool = new HashMap<>();
+ queryOptionWithoutPreferredPool.put("someOtherOption", "value");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithoutPreferredPool),
+ "preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption does not contain
ORDERED_PREFERRED_POOLS");
- // Test case 4: queryOption contains ORDERED_PREFERRED_REPLICAS
+ // Test case 4: queryOption contains ORDERED_PREFERRED_POOLS
+ Map<String, String> queryOptionWithPreferredPool = new HashMap<>();
+ queryOptionWithPreferredPool.put("orderedPreferredPools", "0");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithPreferredPool),
"preferredPoolOptSet",
+ "Should return preferredPoolOptSet when queryOption contains
ORDERED_PREFERRED_POOLS");
+
+ // Test case 5: queryOption contains ORDERED_PREFERRED_REPLICAS
Map<String, String> queryOptionWithPreferredGroup = new HashMap<>();
Review Comment:
`Pool`?
##########
pinot-common/src/test/java/org/apache/pinot/common/metrics/BrokerMetricsTest.java:
##########
@@ -30,27 +30,33 @@
public class BrokerMetricsTest {
@Test
- public void testGetTagForPreferredGroup() {
+ public void testGetTagForPreferredPool() {
// Test case 1: queryOption is null
- assertEquals(BrokerMetrics.getTagForPreferredGroup(null),
"preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption is null");
+ assertEquals(BrokerMetrics.getTagForPreferredPool(null),
"preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption is null");
// Test case 2: queryOption is empty
Map<String, String> emptyQueryOption = new HashMap<>();
- assertEquals(BrokerMetrics.getTagForPreferredGroup(emptyQueryOption),
"preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption is empty");
+ assertEquals(BrokerMetrics.getTagForPreferredPool(emptyQueryOption),
"preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption is empty");
- // Test case 3: queryOption does not contain ORDERED_PREFERRED_REPLICAS
- Map<String, String> queryOptionWithoutPreferredGroup = new HashMap<>();
- queryOptionWithoutPreferredGroup.put("someOtherOption", "value");
-
assertEquals(BrokerMetrics.getTagForPreferredGroup(queryOptionWithoutPreferredGroup),
- "preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption does not
contain ORDERED_PREFERRED_REPLICAS");
+ // Test case 3: queryOption does not contain ORDERED_PREFERRED_POOLS
+ Map<String, String> queryOptionWithoutPreferredPool = new HashMap<>();
+ queryOptionWithoutPreferredPool.put("someOtherOption", "value");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithoutPreferredPool),
+ "preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption does not contain
ORDERED_PREFERRED_POOLS");
- // Test case 4: queryOption contains ORDERED_PREFERRED_REPLICAS
+ // Test case 4: queryOption contains ORDERED_PREFERRED_POOLS
+ Map<String, String> queryOptionWithPreferredPool = new HashMap<>();
+ queryOptionWithPreferredPool.put("orderedPreferredPools", "0");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithPreferredPool),
"preferredPoolOptSet",
+ "Should return preferredPoolOptSet when queryOption contains
ORDERED_PREFERRED_POOLS");
+
+ // Test case 5: queryOption contains ORDERED_PREFERRED_REPLICAS
Map<String, String> queryOptionWithPreferredGroup = new HashMap<>();
queryOptionWithPreferredGroup.put("orderedPreferredReplicas", "0");
Review Comment:
Is the key correct?
##########
pinot-common/src/test/java/org/apache/pinot/common/metrics/BrokerMetricsTest.java:
##########
@@ -30,27 +30,33 @@
public class BrokerMetricsTest {
@Test
- public void testGetTagForPreferredGroup() {
+ public void testGetTagForPreferredPool() {
// Test case 1: queryOption is null
- assertEquals(BrokerMetrics.getTagForPreferredGroup(null),
"preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption is null");
+ assertEquals(BrokerMetrics.getTagForPreferredPool(null),
"preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption is null");
// Test case 2: queryOption is empty
Map<String, String> emptyQueryOption = new HashMap<>();
- assertEquals(BrokerMetrics.getTagForPreferredGroup(emptyQueryOption),
"preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption is empty");
+ assertEquals(BrokerMetrics.getTagForPreferredPool(emptyQueryOption),
"preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption is empty");
- // Test case 3: queryOption does not contain ORDERED_PREFERRED_REPLICAS
- Map<String, String> queryOptionWithoutPreferredGroup = new HashMap<>();
- queryOptionWithoutPreferredGroup.put("someOtherOption", "value");
-
assertEquals(BrokerMetrics.getTagForPreferredGroup(queryOptionWithoutPreferredGroup),
- "preferredGroupOptUnset",
- "Should return preferredGroupOptUnset when queryOption does not
contain ORDERED_PREFERRED_REPLICAS");
+ // Test case 3: queryOption does not contain ORDERED_PREFERRED_POOLS
+ Map<String, String> queryOptionWithoutPreferredPool = new HashMap<>();
+ queryOptionWithoutPreferredPool.put("someOtherOption", "value");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithoutPreferredPool),
+ "preferredPoolOptUnset",
+ "Should return preferredPoolOptUnset when queryOption does not contain
ORDERED_PREFERRED_POOLS");
- // Test case 4: queryOption contains ORDERED_PREFERRED_REPLICAS
+ // Test case 4: queryOption contains ORDERED_PREFERRED_POOLS
+ Map<String, String> queryOptionWithPreferredPool = new HashMap<>();
+ queryOptionWithPreferredPool.put("orderedPreferredPools", "0");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithPreferredPool),
"preferredPoolOptSet",
+ "Should return preferredPoolOptSet when queryOption contains
ORDERED_PREFERRED_POOLS");
+
+ // Test case 5: queryOption contains ORDERED_PREFERRED_REPLICAS
Map<String, String> queryOptionWithPreferredGroup = new HashMap<>();
queryOptionWithPreferredGroup.put("orderedPreferredReplicas", "0");
-
assertEquals(BrokerMetrics.getTagForPreferredGroup(queryOptionWithPreferredGroup),
"preferredGroupOptSet",
- "Should return preferredGroupOptSet when queryOption contains
ORDERED_PREFERRED_REPLICAS");
+
assertEquals(BrokerMetrics.getTagForPreferredPool(queryOptionWithPreferredGroup),
"preferredPoolOptSet",
+ "Should return preferredPoolOptSet when queryOption contains
ORDERED_PREFERRED_POOLS");
Review Comment:
(format) Seems wrong format
--
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]
