Jackie-Jiang commented on code in PR #19269:
URL: https://github.com/apache/pinot/pull/19269#discussion_r3790770767
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java:
##########
@@ -356,14 +369,18 @@ public static RoaringBitmap
getValidDocIdFromServerMatchingCrc(String tableNameW
serverSegmentMetadataReader.getValidDocIdsBitmapFromServer(tableNameWithType,
segmentName, endpoint,
validDocIdsType, 60_000);
} catch (Exception e) {
+ String errorMessage =
+ "Unable to retrieve validDocIds bitmap for segment: " +
segmentName + " from endpoint: " + endpoint;
if (consensusMode == MinionConstants.ValidDocIdsConsensusMode.UNSAFE) {
- LOGGER.warn(
- "Unable to retrieve validDocIds bitmap for segment: " +
segmentName + " from endpoint: " + endpoint, e);
+ LOGGER.warn(errorMessage, e);
continue;
- } else {
- throw new IllegalStateException(
- "Unable to retrieve validDocIds bitmap for segment: " +
segmentName + " from endpoint: " + endpoint, e);
}
+ if (e instanceof NotFoundException) {
+ // Preserve the type: a 404 means the server has no validDocIds for
the segment (e.g. a not-yet-written
+ // snapshot), which callers may handle with a documented fallback,
unlike infrastructure failures.
+ throw new NotFoundException(errorMessage, e);
Review Comment:
The executors' behavior is intentionally out of scope. Neither
`UpsertCompactionTaskExecutor` nor `UpsertCompactMergeTaskExecutor` ever had a
`NotFoundException` fallback (before or after #17696), and for upsert
compaction fail-and-retry on a missing snapshot is the safer semantic: an
all-docs-valid fallback would produce a pointless uncompacted rewrite (no
invalid docs removed, new CRC uploaded), while the failed task simply retries
and converges once the snapshot is persisted — the same fail-closed posture
#17696 chose. Adding a fallback there would be a compaction behavior change for
a separate discussion.
The fallback this PR restores lives on the plugin surface: `MinionTaskUtils`
is used by task executors outside this repo, and the breakage was found in
exactly such a downstream executor whose documented `catch (NotFoundException)`
fallback silently became dead code once the 404 was wrapped — turning a
designed graceful path into a permanent retry loop. Preserving the type (with
segment/endpoint context and the cause) lets such callers dispatch on the
condition again without digging through cause chains; in-repo callers are
unaffected since they catch nothing.
--
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]
