xiangfu0 commented on code in PR #20015:
URL: https://github.com/apache/pinot/pull/20015#discussion_r4363749203
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -115,9 +117,32 @@ public void setUp()
setUpTable();
waitForAllDocsLoaded(60_000);
+ waitForNoOptionalSegments(60_000);
Review Comment:
Worth noting: subclasses that call `super.setUp()` (e.g.
CodecPipelineRealtimeIntegrationTest) inherit this wait, but
MultiTopicRealtimeClusterIntegrationTest reimplements setUp without super and
so skips it. That one is realtime-only where optional segments are less of a
concern, so this is just a heads-up, not a blocker.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -115,9 +117,32 @@ public void setUp()
setUpTable();
waitForAllDocsLoaded(60_000);
+ waitForNoOptionalSegments(60_000);
LOGGER.warn("Finished setting up integration test class: {}",
getClass().getSimpleName());
}
+ /// Waits until the broker routes no optional segment for the table.
+ ///
+ /// The broker routes a new segment as optional until it sees the segment
ONLINE in the external view. The
+ /// single-stage engine still sends optional segments to the servers, which
query them once loaded, so
+ /// [#waitForAllDocsLoaded] can pass before the broker sees the last segment
ONLINE. The multi-stage engine skips
+ /// optional segments, so a multi-stage query issued at that point misses
them.
+ protected void waitForNoOptionalSegments(long timeoutMs) {
+ String routingTableUrl = getBrokerBaseApiUrl() +
"/debug/routingTableWithOptionalSegments/" + getTableName();
+ TestUtils.waitForCondition(() -> {
+ // Routing table per table type: server -> {"segments": [...],
"optionalSegments": [...]}
+ for (JsonNode serverToSegments :
JsonUtils.stringToJsonNode(sendGetRequest(routingTableUrl))) {
+ for (JsonNode segments : serverToSegments) {
+ if (!segments.get("optionalSegments").isEmpty()) {
Review Comment:
Confirmed this is safe: the broker always builds `SegmentsToQuery` with a
non-null `new ArrayList<>()` for optional segments (BaseBrokerRoutingManager),
so Jackson always emits `optionalSegments` and `get(...)` never returns a bare
null here. Even a null value would serialize to NullNode, whose `isEmpty()`
returns true rather than NPEing. Nice.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -115,9 +117,32 @@ public void setUp()
setUpTable();
waitForAllDocsLoaded(60_000);
+ waitForNoOptionalSegments(60_000);
LOGGER.warn("Finished setting up integration test class: {}",
getClass().getSimpleName());
}
+ /// Waits until the broker routes no optional segment for the table.
+ ///
+ /// The broker routes a new segment as optional until it sees the segment
ONLINE in the external view. The
+ /// single-stage engine still sends optional segments to the servers, which
query them once loaded, so
+ /// [#waitForAllDocsLoaded] can pass before the broker sees the last segment
ONLINE. The multi-stage engine skips
+ /// optional segments, so a multi-stage query issued at that point misses
them.
+ protected void waitForNoOptionalSegments(long timeoutMs) {
+ String routingTableUrl = getBrokerBaseApiUrl() +
"/debug/routingTableWithOptionalSegments/" + getTableName();
+ TestUtils.waitForCondition(() -> {
+ // Routing table per table type: server -> {"segments": [...],
"optionalSegments": [...]}
+ for (JsonNode serverToSegments :
JsonUtils.stringToJsonNode(sendGetRequest(routingTableUrl))) {
+ for (JsonNode segments : serverToSegments) {
+ if (!segments.get("optionalSegments").isEmpty()) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }, 100L, timeoutMs, "Broker still routes optional segments for table: " +
getTableName(),
+ Duration.ofMillis(timeoutMs / 10));
Review Comment:
Timeout math looks reasonable: 100ms poll with a 6s log interval over 60s
won't mask a genuine hang, and it uses the exception-catching overload so a
transient 404 before routing is populated just retries instead of failing. Good.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -115,9 +117,32 @@ public void setUp()
setUpTable();
waitForAllDocsLoaded(60_000);
+ waitForNoOptionalSegments(60_000);
LOGGER.warn("Finished setting up integration test class: {}",
getClass().getSimpleName());
}
+ /// Waits until the broker routes no optional segment for the table.
+ ///
+ /// The broker routes a new segment as optional until it sees the segment
ONLINE in the external view. The
+ /// single-stage engine still sends optional segments to the servers, which
query them once loaded, so
+ /// [#waitForAllDocsLoaded] can pass before the broker sees the last segment
ONLINE. The multi-stage engine skips
+ /// optional segments, so a multi-stage query issued at that point misses
them.
+ protected void waitForNoOptionalSegments(long timeoutMs) {
+ String routingTableUrl = getBrokerBaseApiUrl() +
"/debug/routingTableWithOptionalSegments/" + getTableName();
+ TestUtils.waitForCondition(() -> {
+ // Routing table per table type: server -> {"segments": [...],
"optionalSegments": [...]}
+ for (JsonNode serverToSegments :
JsonUtils.stringToJsonNode(sendGetRequest(routingTableUrl))) {
+ for (JsonNode segments : serverToSegments) {
+ if (!segments.get("optionalSegments").isEmpty()) {
+ return false;
+ }
+ }
+ }
+ return true;
+ }, 100L, timeoutMs, "Broker still routes optional segments for table: " +
getTableName(),
+ Duration.ofMillis(timeoutMs / 10));
Review Comment:
Timeout math looks reasonable: 100ms poll with a 6s log interval over 60s
won't mask a genuine hang, and it uses the exception-catching overload so a
transient 404 before routing is populated just retries instead of failing. Good.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -115,9 +117,32 @@ public void setUp()
setUpTable();
waitForAllDocsLoaded(60_000);
+ waitForNoOptionalSegments(60_000);
LOGGER.warn("Finished setting up integration test class: {}",
getClass().getSimpleName());
}
+ /// Waits until the broker routes no optional segment for the table.
+ ///
+ /// The broker routes a new segment as optional until it sees the segment
ONLINE in the external view. The
+ /// single-stage engine still sends optional segments to the servers, which
query them once loaded, so
+ /// [#waitForAllDocsLoaded] can pass before the broker sees the last segment
ONLINE. The multi-stage engine skips
+ /// optional segments, so a multi-stage query issued at that point misses
them.
+ protected void waitForNoOptionalSegments(long timeoutMs) {
+ String routingTableUrl = getBrokerBaseApiUrl() +
"/debug/routingTableWithOptionalSegments/" + getTableName();
+ TestUtils.waitForCondition(() -> {
+ // Routing table per table type: server -> {"segments": [...],
"optionalSegments": [...]}
+ for (JsonNode serverToSegments :
JsonUtils.stringToJsonNode(sendGetRequest(routingTableUrl))) {
+ for (JsonNode segments : serverToSegments) {
+ if (!segments.get("optionalSegments").isEmpty()) {
Review Comment:
Confirmed this is safe: the broker always builds `SegmentsToQuery` with a
non-null `new ArrayList<>()` for optional segments (BaseBrokerRoutingManager),
so Jackson always emits `optionalSegments` and `get(...)` never returns a bare
null here. Even a null value would serialize to NullNode, whose `isEmpty()`
returns true rather than NPEing. Nice.
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/custom/CustomDataQueryClusterIntegrationTest.java:
##########
@@ -115,9 +117,32 @@ public void setUp()
setUpTable();
waitForAllDocsLoaded(60_000);
+ waitForNoOptionalSegments(60_000);
Review Comment:
Worth noting: subclasses that call `super.setUp()` (e.g.
CodecPipelineRealtimeIntegrationTest) inherit this wait, but
MultiTopicRealtimeClusterIntegrationTest reimplements setUp without super and
so skips it. That one is realtime-only where optional segments are less of a
concern, so this is just a heads-up, not a blocker.
--
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]
