You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "krishan1390 (via GitHub)" <gi...@apache.org> on 2026/05/18 11:35:58 UTC

[PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

krishan1390 opened a new pull request, #18518:
URL: https://github.com/apache/pinot/pull/18518

   ## Why this change is needed
   
   `PinotHelixResourceManager.deleteSegments` historically had **zero lineage awareness**. The per-table `SegmentLineage` znode tracks segment replacements (`segmentsFrom -> segmentsTo`) across three states (`IN_PROGRESS`, `COMPLETED`, `REVERTED`), but every external delete path — REST DELETEs, `RetentionManager`, minion task generators, push-failure cleanup — could remove a segment regardless of whether it participated in a live lineage entry. Symmetrically, `endReplaceSegments` only checked that `segmentsTo` are ONLINE; it never re-validated that `segmentsFrom` still existed.
   
   This produced four concrete failure modes:
   
   1. `startReplaceSegments(A1 -> B)` races with a concurrent `deleteSegment(A1)` and commits an `IN_PROGRESS` entry referencing a now-missing `A1`. `revertReplaceSegments` then fails its own existence check without `forceRevert=true`.
   2. Lineage `IN_PROGRESS` with `A1` in `segmentsFrom` — `deleteSegment(A1)` silently breaks the revert path.
   3. Lineage `COMPLETED` with `A1` in `segmentsFrom` — `A1` should only be deleted by the lineage cleanup path (`DefaultLineageManager.updateLineageForRetention`). An external delete bypasses retention/revert semantics.
   4. Lineage `IN_PROGRESS` with `B` in `segmentsTo` — `deleteSegment(B)` makes `endReplaceSegments` hang and then fail on the 10-minute ONLINE poll.
   
   ## The blocked-set rule
   
   A segment `s` is "lineage-locked" iff some lineage entry has:
   
   - state `IN_PROGRESS` and `s ∈ (segmentsFrom ∪ segmentsTo)`, OR
   - state `COMPLETED` and `s ∈ segmentsFrom`.
   
   `REVERTED` entries lock nothing. This is the same set that `SegmentLineageUtils.filterSegmentsBasedOnLineageInPlace` already removes from query routing — i.e. anything currently hidden from queries because of lineage is also blocked from external deletion. The new `SegmentLineageUtils.getDeleteBlockedSegments` helper lives next to that filter so the symmetry is obvious.
   
   ## The correctness mechanism
   
   The change is intentionally asymmetric and avoids any new ZK lock or CAS on the delete hot path. The lineage znode is the globally-visible coordination point, and correctness is enforced by checks at **both** ends of the lifecycle:
   
   | Interleaving | Outcome |
   |---|---|
   | Lineage write commits first; delete reads lineage after | `deleteSegments` sees the entry, fails with `SegmentsInLineageException` (REST 409). IdealState is left untouched. |
   | Delete commits IdealState removal first; lineage write commits after | Both commit. `endReplaceSegments`'s new re-validation of `segmentsFrom` against IdealState fails with an actionable error. Stranded `IN_PROGRESS` entry; recovered via `revertReplaceSegments(forceRevert=true)`. |
   | Truly concurrent reads | Same as above — `endReplaceSegments` catches it. |
   
   The remaining race window is small **and** recoverable: it produces a stranded `IN_PROGRESS` entry, never silent corruption. This applies uniformly to offline and realtime tables because both flow through the same `PinotHelixResourceManager.deleteSegments` primitive.
   
   ## Changes
   
   ### `PinotHelixResourceManager`
   - New private `deleteSegmentsInternal(table, segments, retentionPeriod, bypassLineageCheck)`; public `deleteSegments(...)` overloads delegate with `bypassLineageCheck=false`.
   - New public `deleteSegmentsForLineageCleanup(...)` overloads delegate with `bypassLineageCheck=true` — reserved for internal callers that have already coordinated with the lineage lifecycle (`startReplaceSegments` proactive cleanup, `revertReplaceSegments` post-revert cleanup, `RetentionManager`'s lineage retention pass).
   - Lineage pre-check runs **before** `HelixHelper.removeSegmentsFromIdealState`. A single ZK read; fast-path on missing znode; fail-closed on ZK error.
   - The whole batch is rejected on any conflict — no partial commit semantics on IdealState.
   - `endReplaceSegments` now re-validates that every segment in `lineageEntry.getSegmentsFrom()` is still present in IdealState before flipping the entry to `COMPLETED`. The authoritative source is IdealState (not segment ZK metadata, which lingers in `SegmentDeletionManager`'s retention window after a delete).
   
   ### `SegmentsInLineageException` (new, package-local typed exception)
   Carries `tableNameWithType` and the set of `blockingSegments`. Mapped by `PinotSegmentRestletResource` to HTTP **409 Conflict**.
   
   ### `RetentionManager`
   - Routes the legitimate lineage cleanup call through the bypass overload.
   - New `removeLineageLockedSegments` helper filters lineage-locked segments out of the offline / realtime / hybrid retention batches before calling `deleteSegments`, so the public check never has to reject a retention batch. Lineage-locked segments are left in place until the lineage retention pass decides to remove them.
   
   ### `ControllerConf`
   - New `controller.lineage.exclusive.delete.enabled` kill switch (default `true`) to fall back to legacy behavior during a rolling upgrade if needed.
   
   ### `ControllerMeter`
   - New `LINEAGE_BLOCKED_DELETE_COUNT` table-scoped meter, incremented when a delete request is rejected.
   
   ## Recovery contract
   
   For the narrow race that produces a stranded `IN_PROGRESS` lineage entry, the documented recovery is:
   
   ```
   revertReplaceSegments(tableNameWithType, lineageEntryId, forceRevert=true, ...)
   ```
   
   `forceRevert=true` bypasses the `segmentsFrom` existence check (already supported); after the revert, `segmentsTo` are cleaned up via the bypass overload and the entry transitions to `REVERTED`. `endReplaceSegments`'s new error message names the missing `segmentsFrom` and points operators at this path.
   
   ## Test plan
   - [ ] `mvn test -pl pinot-controller -Dtest=LineageDeleteExclusionTest` (functional coverage for all blocked / allowed states, batch rejection, bypass overload, ZK fast-path, and the `endReplaceSegments` recovery flow)
   - [ ] `mvn test -pl pinot-common -Dtest=SegmentLineageUtilsTest` (unit coverage for the new helper)
   - [ ] `mvn test -pl pinot-controller -Dtest=SegmentLineageCleanupTest,PinotHelixResourceManagerStatelessTest,RetentionManagerTest` (regression coverage on existing lineage / retention paths)
   - [ ] Manual: with `controller.lineage.exclusive.delete.enabled=false`, confirm legacy behavior; re-enable, confirm new 409 path
   - [ ] Manual: trigger an `endReplaceSegments` after a bypass-deleted `segmentsFrom` and confirm the error message names the missing segments and points at `forceRevert=true`


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #18518:
URL: https://github.com/apache/pinot/pull/18518#discussion_r3264962458


##########
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) {
+      return Set.of();
+    }
+    Set<String> blocked = new HashSet<>();
+    for (LineageEntry lineageEntry : segmentLineage.getLineageEntries().values()) {
+      switch (lineageEntry.getState()) {
+        case IN_PROGRESS:
+          blocked.addAll(lineageEntry.getSegmentsFrom());
+          blocked.addAll(lineageEntry.getSegmentsTo());
+          break;
+        case COMPLETED:

Review Comment:
   Why do we include COMPLETED here ? IN_PROGRESS is what we want to exclude right



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #18518:
URL: https://github.com/apache/pinot/pull/18518#discussion_r3265697379


##########
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) {
+      return Set.of();
+    }
+    Set<String> blocked = new HashSet<>();
+    for (LineageEntry lineageEntry : segmentLineage.getLineageEntries().values()) {
+      switch (lineageEntry.getState()) {
+        case IN_PROGRESS:
+          blocked.addAll(lineageEntry.getSegmentsFrom());
+          blocked.addAll(lineageEntry.getSegmentsTo());
+          break;
+        case COMPLETED:

Review Comment:
   > If Seg1 is replaced by Seg2 via lineage, and If we don't include COMPLETED here, then the delete API returns success and thus the client assumes the delete is succesful but in reality it is not (the deleted data is served by Seg2)
   > 
   > I can make changes such that forceDelete can delete segments in FROM list of COMPLETED lineage. But that requires many changes because delete API doesn't take any such parameter today.
   > 
   > Adhoc, these segments can be deleted by triggering RetentionManager.
   > 
   > But I think it makes sense for default behavioir to fail if its part of FROM list of COMPLETED lineage.
   > 
   > Let me know your thoughts
   
   It makes sense to have deterministic behavior. Triggering retention manager is the only way to clean up lineages.  



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18518:
URL: https://github.com/apache/pinot/pull/18518#issuecomment-4477965996

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18518?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `14.28571%` with `12 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 55.78%. Comparing base ([`bd76ff0`](https://app.codecov.io/gh/apache/pinot/commit/bd76ff0ee27f5554a4021e3c2a7ec769eb02617d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1f32387`](https://app.codecov.io/gh/apache/pinot/commit/1f32387f0539c34171b074dd172bd3f204413d5a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 16 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18518?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...ache/pinot/common/lineage/SegmentLineageUtils.java](https://app.codecov.io/gh/apache/pinot/pull/18518?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Flineage%2FSegmentLineageUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbGluZWFnZS9TZWdtZW50TGluZWFnZVV0aWxzLmphdmE=) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18518?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (bd76ff0) and HEAD (1f32387). Click for more details.
   > 
   > <details><summary>HEAD has 16 uploads less than BASE</summary>
   >
   >| Flag | BASE (bd76ff0) | HEAD (1f32387) |
   >|------|------|------|
   >|java-21|5|1|
   >|unittests|2|1|
   >|temurin|5|1|
   >|unittests2|1|0|
   >|integration|3|0|
   >|integration1|1|0|
   >|integration2|1|0|
   >|custom-integration1|1|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #18518      +/-   ##
   ============================================
   - Coverage     63.68%   55.78%   -7.91%     
   + Complexity     1685      815     -870     
   ============================================
     Files          3266     2561     -705     
     Lines        199821   148545   -51276     
     Branches      31022    24015    -7007     
   ============================================
   - Hits         127257    82866   -44391     
   + Misses        62425    58581    -3844     
   + Partials      10139     7098    -3041     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration1](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration2](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-21](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.78% <14.28%> (-7.91%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.78% <14.28%> (-7.91%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.78% <14.28%> (-7.90%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.78% <14.28%> (-0.05%)` | :arrow_down: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18518/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18518?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #18518:
URL: https://github.com/apache/pinot/pull/18518#discussion_r3265041626


##########
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) {
+      return Set.of();
+    }
+    Set<String> blocked = new HashSet<>();
+    for (LineageEntry lineageEntry : segmentLineage.getLineageEntries().values()) {
+      switch (lineageEntry.getState()) {
+        case IN_PROGRESS:
+          blocked.addAll(lineageEntry.getSegmentsFrom());
+          blocked.addAll(lineageEntry.getSegmentsTo());
+          break;
+        case COMPLETED:

Review Comment:
   If Seg1 is replaced by Seg2 via lineage, and If we don't include COMPLETED here, then the delete API returns success and thus the client assumes the delete is succesful but in reality it is not (the deleted data is served by Seg2)
   
   I can make changes such that forceDelete can delete segments in FROM list of COMPLETED lineage. But that requires many changes because delete API doesn't take any such parameter today.
   
   Adhoc, these segments can be deleted by triggering RetentionManager. 
   
   But I think it makes sense for default behavioir to fail if its part of FROM list of COMPLETED lineage. 
   
   Let me know your thoughts



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "krishan1390 (via GitHub)" <gi...@apache.org>.
krishan1390 commented on code in PR #18518:
URL: https://github.com/apache/pinot/pull/18518#discussion_r3265041626


##########
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) {
+      return Set.of();
+    }
+    Set<String> blocked = new HashSet<>();
+    for (LineageEntry lineageEntry : segmentLineage.getLineageEntries().values()) {
+      switch (lineageEntry.getState()) {
+        case IN_PROGRESS:
+          blocked.addAll(lineageEntry.getSegmentsFrom());
+          blocked.addAll(lineageEntry.getSegmentsTo());
+          break;
+        case COMPLETED:

Review Comment:
   If Seg1 is replaced by Seg2 via lineage, and If we don't include COMPLETED here, then the delete API returns success and thus the client assumes the delete is succesful but in reality it is not (the deleted data is served by Seg2)
   
   I can make changes such that forceDelete can delete segments in FROM list of COMPLETED lineage. 
   
   But I think it makes sense for default behavioir to fail if its part of FROM list of COMPLETED lineage. 
   
   Let me know your thoughts



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18518:
URL: https://github.com/apache/pinot/pull/18518#issuecomment-4500406093

   Docs PR opened: pinot-contrib/pinot-docs#809
   
   https://github.com/pinot-contrib/pinot-docs/pull/809


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Enforce mutual exclusion between segment deletion and segment lineage [pinot]

Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish merged PR #18518:
URL: https://github.com/apache/pinot/pull/18518


-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org