Copilot commented on code in PR #19136:
URL: https://github.com/apache/pinot/pull/19136#discussion_r3715535662
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/refreshsegment/RefreshSegmentTaskExecutor.java:
##########
@@ -142,8 +143,11 @@ protected SegmentConversionResult convert(PinotTaskConfig
pinotTaskConfig, File
// Refresh the segment. Segment reload is achieved by generating a new
segment from scratch using the updated schema
// and table configs.
+ // Load with the table-config-derived IndexLoadingConfig so column readers
configured via the table config are
+ // honored (needPreprocess=false: read-only).
+ ImmutableSegment segment = ImmutableSegmentLoader.load(indexDir,
indexLoadingConfig, false);
Review Comment:
The inline comment conflates `needPreprocess=false` with “read-only”.
`needPreprocess` describes whether the loader runs preprocessing steps; it
doesn’t inherently imply immutability/read-only semantics. Suggest rewording to
accurately describe the boolean argument (e.g., “don’t preprocess while
loading”) to prevent future misunderstandings.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/refreshsegment/RefreshSegmentTaskExecutor.java:
##########
@@ -152,6 +156,8 @@ protected SegmentConversionResult convert(PinotTaskConfig
pinotTaskConfig, File
_eventObserver.notifyProgress(pinotTaskConfig,
"Segment processing stats - incomplete rows:" +
driver.getIncompleteRowsFound() + ", dropped rows:"
+ driver.getSkippedRowsFound() + ", sanitized rows:" +
driver.getSanitizedRowsFound());
+ } finally {
+ segment.destroy();
Review Comment:
`segment.destroy()` in a `finally` can mask an in-flight exception if
`destroy()` throws. Consider guarding cleanup so the original failure isn’t
overridden (e.g., wrap `destroy()` in its own try/catch and add the cleanup
error as suppressed, or use try-with-resources if `ImmutableSegment` supports
it). This improves debuggability and reliability during refresh failures.
--
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]
