xiangfu0 commented on code in PR #18583:
URL: https://github.com/apache/pinot/pull/18583#discussion_r3330194147
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/lineage/DefaultLineageManager.java:
##########
@@ -122,24 +121,17 @@ public void updateLineageForRetention(TableConfig
tableConfig, SegmentLineage li
/**
* Helper function to decide whether we should delete segmentsFrom (replaced
segments) given a lineage entry.
*
- * The replaced segments are safe to delete if either:
- * 1) The table is not "REFRESH" (e.g. "APPEND"), in which case they are
deleted immediately, or
- * 2) The lineage entry has been in "COMPLETED" state for longer than {@code
replacedSegmentsRetentionMs}
- * (configurable via {@code replacedSegmentsRetentionPeriod} in table
config, defaulting to 1 day).
+ * The replaced segments are safe to delete once the lineage entry has been
in "COMPLETED" state for longer
+ * than {@code replacedSegmentsRetentionMs} (configurable via {@code
replacedSegmentsRetentionPeriod} in
+ * table config, defaulting to 1 day). The retention period applies
uniformly to all batch ingestion
+ * types — any replacement protocol (REFRESH-table snapshot replace,
APPEND-table minion-driven replace,
+ * segment-group merge) gets the same configurable grace window before its
replaced segments are dropped.
*
- * @param tableConfig a table config
* @param lineageEntry lineage entry
* @param replacedSegmentsRetentionMs configured retention in ms for
replaced segments
* @return True if we can safely delete the replaced segments. False
otherwise.
*/
- private boolean shouldDeleteReplacedSegments(TableConfig tableConfig,
LineageEntry lineageEntry,
- long replacedSegmentsRetentionMs) {
- // TODO: Currently, we preserve the replaced segments for REFRESH tables
only. Once we support
- // data rollback for APPEND tables, we should remove this check.
- String batchSegmentIngestionType =
IngestionConfigUtils.getBatchSegmentIngestionType(tableConfig);
- if (!batchSegmentIngestionType.equalsIgnoreCase("REFRESH")) {
- return true;
- }
+ private boolean shouldDeleteReplacedSegments(LineageEntry lineageEntry, long
replacedSegmentsRetentionMs) {
// Strict < means a 0ms retention won't delete on the exact same
millisecond; this is intentional to
// avoid edge-case races and is consistent with the existing behavior for
non-zero retention values.
return lineageEntry.getTimestamp() < (System.currentTimeMillis() -
replacedSegmentsRetentionMs);
Review Comment:
This changes the default behavior for every non-REFRESH replacement flow,
not just tables that explicitly configured `replacedSegmentsRetentionPeriod`.
Because the default is still 1 day, existing APPEND / merge workflows that
previously cleaned up replaced segments immediately will now retain them for a
day unless operators know to set `0s`. That is a backward-incompatible behavior
change for deployed tables; can we preserve the legacy immediate-cleanup path
unless the table explicitly opts into this retention?
--
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]
