jtao15 commented on a change in pull request #7744:
URL: https://github.com/apache/pinot/pull/7744#discussion_r747245432
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2945,10 +2971,11 @@ public void revertReplaceSegments(String
tableNameWithType, String segmentLineag
// We do not allow to revert the lineage entry with 'REVERTED'
state. For 'IN_PROGRESS", we only allow to
// revert when 'forceRevert' is set to true.
if (lineageEntry.getState() != LineageEntryState.IN_PROGRESS ||
!forceRevert) {
Review comment:
This line can be merged with `L2970` so it aligns with the comment
better?
```suggestion
if (lineageEntry.getState() == LineageEntryState.REVERTED ||
(lineageEntry.getState() == LineageEntryState.IN_PROGRESS && !forceRevert)) {
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2784,32 +2786,55 @@ public String startReplaceSegments(String
tableNameWithType, List<String> segmen
Preconditions.checkArgument(segmentLineage.getLineageEntry(segmentLineageEntryId)
== null,
String.format("SegmentLineageEntryId (%s) already exists in the
segment lineage.", segmentLineageEntryId));
+ List<String> segmentsToCleanUp = new ArrayList<>();
for (String entryId : segmentLineage.getLineageEntryIds()) {
LineageEntry lineageEntry = segmentLineage.getLineageEntry(entryId);
- // If segment entry is in 'REVERTED' state, no need to check for
'segmentsFrom'.
- if (lineageEntry.getState() != LineageEntryState.REVERTED) {
+ // If the lineage entry is in 'REVERTED' state, no need to go
through the validation because we can regard
+ // the entry as not existing.
+ if (lineageEntry.getState() == LineageEntryState.REVERTED) {
+ continue;
+ }
+
+ // By here, the lineage entry is either 'IN_PROGRESS' or 'COMPLETED'.
+ if (forceCleanup && lineageEntry.getState() ==
LineageEntryState.IN_PROGRESS && CollectionUtils
+ .isEqualCollection(segmentsFrom,
lineageEntry.getSegmentsFrom())) {
+ // When 'forceCleanup' is enabled, we need to proactively revert
the lineage entry when we find the lineage
+ // entry with the same 'segmentFrom' values.
+ segmentLineage.updateLineageEntry(entryId,
Review comment:
We need to check if the `segmentsFrom` are online before updating the
status as `REVERTED`.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2971,6 +2998,11 @@ public void revertReplaceSegments(String
tableNameWithType, String segmentLineag
// routing table because it is possible that there has been no EV
change but the routing result may be
// different after updating the lineage entry.
sendRoutingTableRebuildMessage(tableNameWithType);
+
+ // Invoke the proactive clean-up for segments that we no longer
needs in case 'forceRevert' is enabled
+ if (forceRevert) {
+ deleteSegments(tableNameWithType, lineageEntry.getSegmentsTo());
Review comment:
Currently we don't have any cleanup logic for `REVERTED` lineage entry.
Also we are removing the segments asynchronously here, it's possible that some
segments in `segmentsTo` are not deleted successfully. We can handle the
cleanup for both in retention manager in following prs.
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -2784,32 +2786,55 @@ public String startReplaceSegments(String
tableNameWithType, List<String> segmen
Preconditions.checkArgument(segmentLineage.getLineageEntry(segmentLineageEntryId)
== null,
String.format("SegmentLineageEntryId (%s) already exists in the
segment lineage.", segmentLineageEntryId));
+ List<String> segmentsToCleanUp = new ArrayList<>();
for (String entryId : segmentLineage.getLineageEntryIds()) {
LineageEntry lineageEntry = segmentLineage.getLineageEntry(entryId);
- // If segment entry is in 'REVERTED' state, no need to check for
'segmentsFrom'.
- if (lineageEntry.getState() != LineageEntryState.REVERTED) {
+ // If the lineage entry is in 'REVERTED' state, no need to go
through the validation because we can regard
+ // the entry as not existing.
+ if (lineageEntry.getState() == LineageEntryState.REVERTED) {
+ continue;
+ }
+
+ // By here, the lineage entry is either 'IN_PROGRESS' or 'COMPLETED'.
+ if (forceCleanup && lineageEntry.getState() ==
LineageEntryState.IN_PROGRESS && CollectionUtils
+ .isEqualCollection(segmentsFrom,
lineageEntry.getSegmentsFrom())) {
+ // When 'forceCleanup' is enabled, we need to proactively revert
the lineage entry when we find the lineage
+ // entry with the same 'segmentFrom' values.
+ segmentLineage.updateLineageEntry(entryId,
Review comment:
We are not reusing the revertReplaceSegment api here because we will
have only one zookeeper update in this way?
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
