You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/11/11 07:37:13 UTC
[GitHub] [pinot] jtao15 commented on a change in pull request #7744: Add forceCleanup option for 'startReplaceSegments' API
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-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