Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wait for full segment commit protocol on force commit #10479

Conversation

jadami10
Copy link
Contributor

This closes #10460

This isn't necessarily a bug, but it can lead to unexpected behavior. We observed that picking the first server to respond to force commit led to a case where a server that was several hours behind was chosen as the winner. In the issue, we discussed that the intention was to kick off committing immediately on force commit, but it should generally not be detrimental (extra ~3 seconds) to wait for the typical segment commit protocol.

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2023

Codecov Report

Merging #10479 (fa1ffc9) into master (0e6c386) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #10479      +/-   ##
============================================
+ Coverage     70.26%   70.27%   +0.01%     
+ Complexity     6113     5274     -839     
============================================
  Files          2070     2070              
  Lines        111951   111950       -1     
  Branches      17051    17050       -1     
============================================
+ Hits          78658    78672      +14     
+ Misses        27746    27729      -17     
- Partials       5547     5549       +2     
Flag Coverage Δ
integration1 24.42% <0.00%> (+0.04%) ⬆️
integration2 24.27% <0.00%> (-0.05%) ⬇️
unittests1 67.86% <ø> (+<0.01%) ⬆️
unittests2 14.01% <0.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
.../helix/core/realtime/SegmentCompletionManager.java 72.30% <0.00%> (+0.14%) ⬆️

... and 45 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jadami10
Copy link
Contributor Author

@Jackie-Jiang , do you mind rerunning the failed workflows? looks like a flake

@Jackie-Jiang Jackie-Jiang merged commit 40ac22d into apache:master Mar 29, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forceCommit, and maybe segment commit in general, might pick an unhealthy/lagging winner
4 participants