Jackie-Jiang commented on code in PR #10350:
URL: https://github.com/apache/pinot/pull/10350#discussion_r1134509046
##########
pinot-broker/src/test/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorTest.java:
##########
@@ -106,12 +139,13 @@ public void testInstanceSelector() {
String offlineTableName = "testTable_OFFLINE";
BrokerMetrics brokerMetrics = mock(BrokerMetrics.class);
AdaptiveServerSelector adaptiveServerSelector = null;
- BalancedInstanceSelector balancedInstanceSelector = new
BalancedInstanceSelector(offlineTableName, brokerMetrics,
- adaptiveServerSelector);
+ ZkHelixPropertyStore<ZNRecord> propertyStore =
mock(ZkHelixPropertyStore.class);
+ BalancedInstanceSelector balancedInstanceSelector =
Review Comment:
This test already contains all types of instance selectors. Do we need 3 new
separate tests for them? We don't want to keep duplicate tests as that will add
management overhead
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -113,18 +114,27 @@ public static class Helix {
public static final String UNTAGGED_MINION_INSTANCE = "minion_untagged";
public static class StateModel {
+ // public only for testing purpose.
+ public static final long NEW_SEGMENT_EXPIRATION_MILLIS =
TimeUnit.MINUTES.toMillis(5);
public static class SegmentStateModel {
public static final String ONLINE = "ONLINE";
public static final String OFFLINE = "OFFLINE";
public static final String ERROR = "ERROR";
public static final String CONSUMING = "CONSUMING";
+ public static boolean isOnline(String state) {
+ return state.equals(ONLINE) || state.equals(CONSUMING);
+ }
}
public static class BrokerResourceStateModel {
public static final String ONLINE = "ONLINE";
public static final String OFFLINE = "OFFLINE";
public static final String ERROR = "ERROR";
}
+
+ public static boolean isNewSegment(long creationTimeMillis, long
nowMillis) {
Review Comment:
This doesn't belong to this class. You may put it in `InstanceSelector`
interface, or add a new util for it
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/adaptiveserverselector/AdaptiveServerSelector.java:
##########
@@ -32,27 +32,29 @@ public interface AdaptiveServerSelector {
* Picks the best server to route a query from the list of candidate servers.
*
* @param serverCandidates Candidate servers from which the best server
should be chosen.
- * @return server identifier
+ * @return server identifier and whether server is online
*/
- String select(List<String> serverCandidates);
+ Pair<String, Boolean> select(List<Pair<String, Boolean>> serverCandidates);
Review Comment:
The ONLINE/OFFLINE is for a single segment server state. Here we should
probably only keep the online servers
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/StrictReplicaGroupInstanceSelector.java:
##########
@@ -58,13 +62,28 @@
* transitioning/error scenario (external view does not match ideal state), if
a segment is down on S1, we mark all
* segments with the same assignment ([S1, S2, S3]) down on S1 to ensure that
we always route the segments to the same
* replica-group.
- * </pre>
+ *
+ * Note that New segment won't be used to exclude instance from serving where
new segment is unavailable.
+ *
+ * </pre>
*/
public class StrictReplicaGroupInstanceSelector extends
ReplicaGroupInstanceSelector {
- public StrictReplicaGroupInstanceSelector(String tableNameWithType,
BrokerMetrics brokerMetrics, @Nullable
- AdaptiveServerSelector adaptiveServerSelector) {
- super(tableNameWithType, brokerMetrics, adaptiveServerSelector);
+ private boolean isNewSegment(String segment, Map<String, SegmentState>
newSegmentStateMap, long nowMillis) {
+ SegmentState state = newSegmentStateMap.get(segment);
+ return state != null && state.isNew(nowMillis);
+ }
+
+ public StrictReplicaGroupInstanceSelector(String tableNameWithType,
BrokerMetrics brokerMetrics,
+ @Nullable AdaptiveServerSelector adaptiveServerSelector,
ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ this(tableNameWithType, brokerMetrics, adaptiveServerSelector,
propertyStore, Clock.systemUTC());
+ }
+
+ // Test only for clock injection.
Review Comment:
(minor) Use `@VisibleForTesting` instead of comment
##########
pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java:
##########
@@ -617,7 +617,8 @@ protected void waitForDocsLoaded(long timeoutMs, boolean
raiseError, String tabl
@Override
public Boolean apply(@Nullable Void aVoid) {
try {
- return getCurrentCountStarResult(tableName) == countStarResult;
+ long curCount = getCurrentCountStarResult(tableName);
Review Comment:
(minor) unnecessary change
##########
pinot-broker/pom.xml:
##########
@@ -153,5 +153,11 @@
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
A lighter library (avoid conflict) to use here:
```
<dependency>
<groupId>com.mercateo</groupId>
<artifactId>test-clock</artifactId>
<version>1.0.2</version>
<scope>test</scope>
</dependency>
```
Let's put it in the root pom with the version, then reference that here
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
