Jackie-Jiang commented on code in PR #18518:
URL: https://github.com/apache/pinot/pull/18518#discussion_r3270358694
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineageUtils.java:
##########
@@ -53,4 +54,35 @@ public static void
filterSegmentsBasedOnLineageInPlace(Set<String> segments, Seg
}
}
}
+
+ /**
Review Comment:
(minor) Switch to markdown javadoc style
##########
pinot-common/src/main/java/org/apache/pinot/common/lineage/SegmentLineageUtils.java:
##########
@@ -53,4 +54,35 @@ public static void
filterSegmentsBasedOnLineageInPlace(Set<String> segments, Seg
}
}
}
+
+ /**
+ * Returns the set of segments that participate in a live lineage entry and
therefore must not be deleted by
+ * external callers.
+ * <ul>
+ * <li>{@code IN_PROGRESS} entries lock both {@code segmentsFrom} and
{@code segmentsTo}.</li>
+ * <li>{@code COMPLETED} entries lock {@code segmentsFrom}</li>
+ * <li>{@code REVERTED} entries lock nothing.</li>
+ * </ul>
+ */
+ public static Set<String> getDeleteBlockedSegments(SegmentLineage
segmentLineage) {
+ if (segmentLineage == null) {
Review Comment:
(nit) This check is redundant. The convention is that if an argument is not
annotated with `@Nullable`, it cannot be `null`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1089,12 +1123,47 @@ public PinotResourceManagerResponse
deleteSegments(String tableNameWithType, Lis
_segmentDeletionManager.deleteSegments(tableNameWithType,
segmentNames, tableConfig);
}
return PinotResourceManagerResponse.success("Segment " + segmentNames +
" deleted");
+ } catch (SegmentsInLineageException e) {
+ LOGGER.warn("Refusing to delete segments from table: {}. {}",
tableNameWithType, e.getMessage());
+ throw e;
} catch (final Exception e) {
LOGGER.error("Caught exception while deleting segment: {} from table:
{}", segmentNames, tableNameWithType, e);
return PinotResourceManagerResponse.failure(e.getMessage());
}
}
+ /**
+ * Reads the current segment lineage znode (if any) and throws {@link
SegmentsInLineageException} when the
+ * delete batch intersects the lineage-locked set.
+ */
+ private void rejectIfTargetsLineageLockedSegments(String tableNameWithType,
List<String> segmentNames) {
+ SegmentLineage segmentLineage =
SegmentLineageAccessHelper.getSegmentLineage(_propertyStore, tableNameWithType);
+ if (segmentLineage == null) {
+ return;
+ }
+ Set<String> blocked =
SegmentLineageUtils.getDeleteBlockedSegments(segmentLineage);
+ if (blocked.isEmpty()) {
+ return;
+ }
+ Set<String> blockingTargets = new LinkedHashSet<>();
Review Comment:
(minor) Track in a `List`?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1089,12 +1123,47 @@ public PinotResourceManagerResponse
deleteSegments(String tableNameWithType, Lis
_segmentDeletionManager.deleteSegments(tableNameWithType,
segmentNames, tableConfig);
}
return PinotResourceManagerResponse.success("Segment " + segmentNames +
" deleted");
+ } catch (SegmentsInLineageException e) {
+ LOGGER.warn("Refusing to delete segments from table: {}. {}",
tableNameWithType, e.getMessage());
+ throw e;
Review Comment:
Why throwing the exception out?
--
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]
