Jackie-Jiang commented on code in PR #19735:
URL: https://github.com/apache/pinot/pull/19735#discussion_r4200510283
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/lineage/DefaultLineageManager.java:
##########
@@ -107,31 +122,38 @@ public void updateLineageForRetention(TableConfig
tableConfig, SegmentLineage li
segmentsToDelete.addAll(sourceSegments);
}
}
- } else if (lineageEntry.getState() == LineageEntryState.REVERTED || (
- lineageEntry.getState() == LineageEntryState.IN_PROGRESS &&
lineageEntry.getTimestamp()
- < System.currentTimeMillis() - lineageCleanupRetentionMs)) {
+ } else if (isZombieEntryEligibleForCleanup(lineageEntry,
lineageCleanupThresholdMs)) {
Review Comment:
[P1] Retire obsolete shared destination references independently of the
grace period.
For a recent `REVERTED A→B` followed by `COMPLETED A→B`, this condition
retains the reverted entry.
`SegmentLineageUtils.filterSegmentsBasedOnLineageInPlace`, used by the broker
preselector, then excludes `B` for the reverted entry and `A` for the completed
entry, so neither is routable despite preserving `B` physically.
The protection can also disappear across passes: with APPEND defaults, a
pass after four hours deletes `A`; the next pass removes the completed entry
because `A` is absent; once the reverted entry reaches 24 hours, `B` is
unprotected and is deleted. Could we prune/retire obsolete shared references
independently of zombie retention, and add a multi-pass regression that checks
broker routing as well as segment preservation?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -4803,16 +4922,35 @@ public void revertReplaceSegments(String
tableNameWithType, String segmentLineag
TableConfig tableConfig =
ZKMetadataProvider.getTableConfig(_propertyStore, tableNameWithType);
Map<String, String> customMap =
revertReplaceSegmentsRequest == null ? null :
revertReplaceSegmentsRequest.getCustomMap();
- if (writeLineageEntryWithLock(tableConfig, segmentLineageEntryId,
lineageEntryToUpdate, lineageEntry,
- _propertyStore, LineageUpdateType.REVERT, customMap)) {
+ int writtenLineageVersion = writeLineageEntryWithLock(tableConfig,
segmentLineageEntryId,
Review Comment:
[P1] Retry lifecycle validation when the cleanup fence advances.
The new fence still permits this interleaving: revert validates that source
`A` of `COMPLETED A→B` is ONLINE; retention cleanup removes `A` and bumps the
lineage version; `writeLineageEntryWithLock()` re-fetches the newer version,
sees unchanged entry contents, and commits `REVERTED`. Routing then returns to
missing `A`, and proactive revert cleanup can also delete `B`. Completion has
the analogous gap for expired `IN_PROGRESS` destinations.
This race existed before the PR, but the new version bump does not close it
because the helper retries only the write. Could we bind it to the version used
for validation and retry the outer operation on a version change, so the
segment-existence checks run again?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java:
##########
@@ -543,9 +553,15 @@ private void
manageSegmentLineageCleanupForTable(TableConfig tableConfig) {
}
// Delete segments based on the segment lineage
if (!segmentsToDelete.isEmpty()) {
-
_pinotHelixResourceManager.deleteSegmentsForLineageCleanup(tableNameWithType,
segmentsToDelete);
- LOGGER.info("Finished cleaning up segment lineage for table: {} in {}ms,
deleted segments: {}",
- tableNameWithType, (System.currentTimeMillis() - cleanupStartTime),
segmentsToDelete);
+ PinotResourceManagerResponse cleanupResponse =
_pinotHelixResourceManager.deleteSegmentsForLineageCleanup(
+ tableNameWithType, segmentsToDelete, cleanupLineageVersion[0]);
+ if (cleanupResponse.isSuccessful()) {
+ LOGGER.info("Finished cleaning up segment lineage for table: {} in
{}ms, deleted segments: {}",
+ tableNameWithType, (System.currentTimeMillis() -
cleanupStartTime), segmentsToDelete);
+ } else {
Review Comment:
[P2] Preserve orphan cleanup intent when the version check rejects deletion.
For an expired zombie destination absent from IdealState but still present
in segment metadata, `DefaultLineageManager` removes its lineage entry and
returns the orphan as a deletion candidate. That removal is committed before
this call. If an unrelated replacement updates lineage in between, the new
version check rejects deletion and this branch only logs it. The next pass
cannot rediscover the candidate because its lineage entry is gone.
OFFLINE REFRESH tables skip ordinary retention, so the metadata and
deep-store data can remain indefinitely. Could we keep durable cleanup intent
until deletion succeeds, with a regression that interleaves an unrelated
lineage update and verifies a later pass still reaps the orphan?
--
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]
