Enhance revertReplaceSegments api by jtao15 · Pull Request #8166 · apache/pinot

jtao15 · 2022-02-08T19:07:02Z

Enhance revertReplaceSegments api so reverting entry1 in the following example is not allowed:
entry1: {Seg_0 -> Seg1, COMPLETED}
entry2: {Seg_1 -> Seg2, COMPLETED/IN_PROGRESS}

richardstartin · 2022-02-08T19:10:40Z

...ller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java

Can you put a fail after line 761 then please?

@jtao15 Can we add fail() for all the places where we try{} catch{} ?

I think that we have a lot of tests like this and I remember that I didn't add fail().

jackjlli · 2022-02-08T19:19:28Z

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

the currentLineageEntry here seems unnecessary?

jackjlli · 2022-02-08T19:19:47Z

...ller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java

snleee

LGTM otherwise

snleee · 2022-02-08T20:07:09Z

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

snleee · 2022-02-08T20:08:11Z

...ller/src/test/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManagerTest.java

@jtao15 Can we add fail() for all the places where we try{} catch{} ?

I think that we have a lot of tests like this and I remember that I didn't add fail().

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

codecov-commenter · 2022-02-09T23:32:39Z

Codecov Report

Merging #8166 (2ce2f88) into master (58f74ce) will increase coverage by 0.04%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master    #8166      +/-   ##
============================================
+ Coverage     71.36%   71.40%   +0.04%     
+ Complexity     4308     4307       -1     
============================================
  Files          1623     1623              
  Lines         84356    84365       +9     
  Branches      12654    12657       +3     
============================================
+ Hits          60197    60245      +48     
+ Misses        20023    19993      -30     
+ Partials       4136     4127       -9

Flag	Coverage Δ
integration1	`28.90% <0.00%> (+0.03%)`	⬆️
integration2	`27.56% <0.00%> (-0.13%)`	⬇️
unittests1	`67.88% <ø> (+<0.01%)`	⬆️
unittests2	`14.19% <90.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...ntroller/helix/core/PinotHelixResourceManager.java	`66.05% <90.00%> (+0.28%)`	⬆️
...pinot/common/utils/fetcher/HttpSegmentFetcher.java	`61.53% <0.00%> (-10.26%)`	⬇️
...a/org/apache/pinot/common/utils/ServiceStatus.java	`60.00% <0.00%> (-7.15%)`	⬇️
.../pinot/server/starter/helix/BaseServerStarter.java	`57.98% <0.00%> (-1.97%)`	⬇️
...r/helix/SegmentOnlineOfflineStateModelFactory.java	`63.20% <0.00%> (-1.89%)`	⬇️
...e/pinot/segment/local/io/util/PinotDataBitSet.java	`95.62% <0.00%> (-1.46%)`	⬇️
...ces/PinotSegmentUploadDownloadRestletResource.java	`57.51% <0.00%> (-1.29%)`	⬇️
...e/pinot/broker/broker/helix/BaseBrokerStarter.java	`75.67% <0.00%> (-1.09%)`	⬇️
.../pinot/core/data/manager/BaseTableDataManager.java	`86.80% <0.00%> (-0.86%)`	⬇️
...e/pinot/common/utils/FileUploadDownloadClient.java	`63.00% <0.00%> (-0.74%)`	⬇️
... and 18 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 58f74ce...2ce2f88. Read the comment docs.

jackjlli

LGTM. Thanks for making the change!

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

…g example is not allowed: entry1: {Seg_0 -> Seg1, COMPLETED} entry2: {Seg_1 -> Seg2, COMPLETED/IN_PROGRESS}

richardstartin reviewed Feb 8, 2022

View reviewed changes

jackjlli reviewed Feb 8, 2022

View reviewed changes

snleee approved these changes Feb 8, 2022

View reviewed changes

jtao15 force-pushed the consistentPush branch from ba79980 to c6f5acd Compare February 9, 2022 23:01

jackjlli approved these changes Feb 10, 2022

View reviewed changes

snleee reviewed Feb 11, 2022

View reviewed changes

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java Outdated Show resolved Hide resolved

jtao15 force-pushed the consistentPush branch 2 times, most recently from 4e9ca3e to 61261e7 Compare February 11, 2022 00:52

Enhance revertReplaceSegments api so reverting entry1 in the followin…

2ce2f88

…g example is not allowed: entry1: {Seg_0 -> Seg1, COMPLETED} entry2: {Seg_1 -> Seg2, COMPLETED/IN_PROGRESS}

jtao15 force-pushed the consistentPush branch from 61261e7 to 2ce2f88 Compare February 11, 2022 00:52

snleee merged commit fb572bd into apache:master Feb 11, 2022

Conversation

jtao15 commented Feb 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snleee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jackjlli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Feb 9, 2022 •

edited

Loading