Add revertSegmentReplacement API #7662

snleee · 2021-10-29T01:36:12Z

When all the segmentsFrom are 'ONLINE' state, this new API allows
to revert the segment replacement and use the original segments
when routing the query. This will be useful when the bad data
gets pushed and wants to go back to the previous state quickly.

codecov-commenter · 2021-10-29T02:13:36Z

Codecov Report

Merging #7662 (3e70968) into master (127f4c3) will decrease coverage by 0.15%.
The diff coverage is 18.75%.

@@             Coverage Diff              @@
##             master    #7662      +/-   ##
============================================
- Coverage     71.67%   71.52%   -0.16%     
- Complexity     3996     4037      +41     
============================================
  Files          1576     1578       +2     
  Lines         80037    80363     +326     
  Branches      11865    11942      +77     
============================================
+ Hits          57366    57479     +113     
- Misses        18805    19003     +198     
- Partials       3866     3881      +15

Flag	Coverage Δ
integration1	`28.96% <12.50%> (-0.36%)`	⬇️
integration2	`27.69% <0.00%> (+0.09%)`	⬆️
unittests1	`68.66% <100.00%> (-0.02%)`	⬇️
unittests2	`14.55% <10.93%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ntroller/helix/core/PinotHelixResourceManager.java	`62.32% <7.89%> (-1.60%)`	⬇️
...ces/PinotSegmentUploadDownloadRestletResource.java	`61.08% <23.52%> (-2.38%)`	⬇️
...troller/helix/core/retention/RetentionManager.java	`82.75% <50.00%> (-0.72%)`	⬇️
...apache/pinot/common/lineage/LineageEntryState.java	`100.00% <100.00%> (ø)`
...data/manager/realtime/SegmentCommitterFactory.java	`64.70% <0.00%> (-29.42%)`	⬇️
...ot/core/query/pruner/ColumnValueSegmentPruner.java	`74.86% <0.00%> (-19.31%)`	⬇️
.../common/request/context/predicate/EqPredicate.java	`66.66% <0.00%> (-13.34%)`	⬇️
...unction/DistinctCountHLLMVAggregationFunction.java	`17.16% <0.00%> (-10.91%)`	⬇️
...nction/DistinctCountBitmapAggregationFunction.java	`41.96% <0.00%> (-10.89%)`	⬇️
...er/api/resources/LLCSegmentCompletionHandlers.java	`53.46% <0.00%> (-8.92%)`	⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 127f4c3...3e70968. Read the comment docs.

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

jackjlli · 2021-10-29T18:28:55Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

+                segmentLineageEntryId));
+
+        // Revert is not allowed if the state is not 'COMPLETED'
+        if (lineageEntry.getState() != LineageEntryState.COMPLETED) {


What if the lineage entry is currently in IN_PROGRESS state, let's say a batch of segment upload is interrupted? Are we still able to mark it to REVERTED?

+1. We should allow explicitly revert IN_PROGRESS lineage (add a flag is fine) in case user knows the the segment upload is already interrupted

good point. I added forceRevert flag so that the user can explicitly revert the lineage entry even if it's IN_PROGRESS.

Jackie-Jiang

Should also modify startReplaceSegments and ignore the segmentsFrom check for REVERTED lineage. User'll likely start another segment replace with the same source segments.

Do we need to modify the broker routing logic? Can we add a test to verify the broker behavior on REVERTED lineage?

...troller/src/main/java/org/apache/pinot/controller/helix/core/retention/RetentionManager.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

Jackie-Jiang · 2021-10-29T23:27:11Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

+                segmentLineageEntryId));
+
+        // Revert is not allowed if the state is not 'COMPLETED'
+        if (lineageEntry.getState() != LineageEntryState.COMPLETED) {


+1. We should allow explicitly revert IN_PROGRESS lineage (add a flag is fine) in case user knows the the segment upload is already interrupted

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

...ava/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

When all the segmentsFrom are 'ONLINE' state, this new API allows to revert the segment replacement and use the original segments when routing the query. This will be useful when the bad data gets pushed and wants to go back to the previous state quickly.

jackjlli

LGTM. Thanks for working on it!

jtao15 · 2021-11-01T23:26:41Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

+        if (lineageEntry.getState() != LineageEntryState.COMPLETED) {
+          // 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) {


Nit: this line can be merged with L2944?

Suggested change

if (lineageEntry.getState() != LineageEntryState.IN_PROGRESS || !forceRevert) {

if (lineageEntry.getState() == LineageEntryState.REVERTED || (lineageEntry.getState() == LineageEntryState.IN_PROGRESS && !forceRevert)) {

oops, I missed your review and I already checked in. I will address this when I file the follow-up PR for proactive deletion.

jtao15 · 2021-11-01T23:47:29Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

+          // 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);
+          return true;


Once the lineage is updated successfully, are we planning to proactively invoke the segment deletion? It will help the case that segmentsTo take a lot of disk spaces and will block segment uploading until retention manager cleans it up.

Correct. I will file the follow-up PRs for proactive segment delete.

snleee · 2021-11-22T22:44:38Z

#7813

snleee force-pushed the add-revert-segment-replacement-protocol branch from d1d8d41 to 4e68a6e Compare October 29, 2021 01:37

snleee requested a review from jackjlli October 29, 2021 01:38

snleee force-pushed the add-revert-segment-replacement-protocol branch from 4e68a6e to 07750bb Compare October 29, 2021 01:41

jackjlli reviewed Oct 29, 2021

View reviewed changes

Jackie-Jiang reviewed Oct 29, 2021

View reviewed changes

snleee force-pushed the add-revert-segment-replacement-protocol branch 4 times, most recently from f0aa01e to 3508902 Compare November 1, 2021 21:02

jackjlli reviewed Nov 1, 2021

View reviewed changes

snleee force-pushed the add-revert-segment-replacement-protocol branch from 3508902 to 3e70968 Compare November 1, 2021 22:40

jackjlli approved these changes Nov 1, 2021

View reviewed changes

jtao15 reviewed Nov 1, 2021

View reviewed changes

snleee merged commit da98e8b into apache:master Nov 2, 2021

snleee deleted the add-revert-segment-replacement-protocol branch November 2, 2021 00:33

kriti-sc pushed a commit to kriti-sc/incubator-pinot that referenced this pull request Dec 12, 2021

Add revertSegmentReplacement API (apache#7662)

95ffb73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add revertSegmentReplacement API #7662

Add revertSegmentReplacement API #7662

snleee commented Oct 29, 2021

codecov-commenter commented Oct 29, 2021 •

edited

Loading

jackjlli Oct 29, 2021

Jackie-Jiang Oct 29, 2021

snleee Nov 1, 2021

Jackie-Jiang left a comment

Jackie-Jiang Oct 29, 2021

jackjlli left a comment

jtao15 Nov 1, 2021

snleee Nov 2, 2021

jtao15 Nov 1, 2021

snleee Nov 2, 2021

snleee commented Nov 22, 2021

	if (lineageEntry.getState() != LineageEntryState.IN_PROGRESS \|\| !forceRevert) {
	if (lineageEntry.getState() == LineageEntryState.REVERTED \|\| (lineageEntry.getState() == LineageEntryState.IN_PROGRESS && !forceRevert)) {

Add revertSegmentReplacement API #7662

Add revertSegmentReplacement API #7662

Conversation

snleee commented Oct 29, 2021

codecov-commenter commented Oct 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackjlli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snleee commented Nov 22, 2021

codecov-commenter commented Oct 29, 2021 •

edited

Loading