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

handle absent segments so that catchup checker doesn't get stuck on them #12883

Merged
merged 6 commits into from
Apr 22, 2024

Conversation

klsince
Copy link
Contributor

@klsince klsince commented Apr 10, 2024

Handle absent segments so that catchup checker doesn't get stuck on them. When server is catching up, the tables or segments might get dropped, if those segments are not skipped, the checker will block server from starting up.

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 57.95455% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 62.13%. Comparing base (59551e4) to head (8d03730).
Report is 350 commits behind head on master.

Files Patch % Lines
.../pinot/server/starter/helix/BaseServerStarter.java 0.00% 33 Missing ⚠️
.../helix/IngestionBasedConsumptionStatusChecker.java 92.45% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #12883      +/-   ##
============================================
+ Coverage     61.75%   62.13%   +0.38%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2502      +66     
  Lines        133233   136547    +3314     
  Branches      20636    21137     +501     
============================================
+ Hits          82274    84840    +2566     
- Misses        44911    45423     +512     
- Partials       6048     6284     +236     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 62.12% <57.95%> (+0.41%) ⬆️
java-21 27.95% <57.95%> (-33.68%) ⬇️
skip-bytebuffers-false 62.12% <57.95%> (+0.37%) ⬆️
skip-bytebuffers-true 27.94% <57.95%> (+0.21%) ⬆️
temurin 62.13% <57.95%> (+0.38%) ⬆️
unittests 62.12% <57.95%> (+0.38%) ⬆️
unittests1 46.66% <ø> (-0.23%) ⬇️
unittests2 27.95% <57.95%> (+0.22%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the extra timeout introduced here because it is not robust.
One solution could be: when we cannot find the table/segment served on the server, read the ideal state again to read the source of truth, then decide whether to skip it

@Jackie-Jiang
Copy link
Contributor

Fix #12381

@klsince klsince force-pushed the handle_absent_segments_catchup_checker branch from b681ee1 to 7b5fa51 Compare April 18, 2024 22:06
@klsince klsince force-pushed the handle_absent_segments_catchup_checker branch from 7b5fa51 to f1a9673 Compare April 18, 2024 22:07
@klsince klsince force-pushed the handle_absent_segments_catchup_checker branch from e9065d4 to e85130c Compare April 19, 2024 18:54
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@klsince klsince merged commit 8e10320 into apache:master Apr 22, 2024
20 checks passed
@klsince klsince deleted the handle_absent_segments_catchup_checker branch April 22, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants