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

Allow optional segments that can be skipped by servers without failing the query #11978

Merged
merged 11 commits into from
Nov 29, 2023

Conversation

klsince
Copy link
Contributor

@klsince klsince commented Nov 8, 2023

This PR tries to address issue: #11965, basically by extending the NewSegment concept added for instanceSelector early on. The NewSegment concept was added to handle the case when new consuming segments are added in IdealState but not online in ExternalView yet, as servers may take time to load new consuming segments and report the status back to Helix. Instead of reporting such new segments as unavailable segments, broker simply skips them.

But simply skipping them at broker side could cause the wrong query results as detailed in the new issue. e.g. for upsert table, the server could start to ingest new records into the new segments and invalidate the records in existing segments, even before the broker could mark those new consuming segments as online, thus queries could not see the new records in new consuming segments during a short window. This issue could cause inconsistent query results, particularly for upsert table.

So in this PR, we track those new segments that not online in ExternalView, and pass them to servers, instead of skipping them at broker side. The servers can skip them if they are not available, otherwise, process they for correct query results, particular for upsert table.

TODO: this PR only made changes for SingleConnectionBrokerRequestHandler. Add support for GrpcServerRequest (based on protobuf) and multi-stage engine in separate PRs

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2023

Codecov Report

Attention: 68 lines in your changes are missing coverage. Please review.

Comparison is base (97f8f5f) 61.52% compared to head (c7606af) 61.61%.
Report is 4 commits behind head on master.

Files Patch % Lines
...e/pinot/broker/api/resources/PinotBrokerDebug.java 0.00% 28 Missing ⚠️
.../pinot/core/data/manager/BaseTableDataManager.java 14.28% 5 Missing and 1 partial ⚠️
...instanceselector/ReplicaGroupInstanceSelector.java 50.00% 5 Missing ⚠️
...roker/requesthandler/GrpcBrokerRequestHandler.java 0.00% 4 Missing ⚠️
...che/pinot/broker/routing/BrokerRoutingManager.java 76.47% 2 Missing and 2 partials ⚠️
...core/query/executor/ServerQueryExecutorV1Impl.java 33.33% 3 Missing and 1 partial ⚠️
...routing/instanceselector/BaseInstanceSelector.java 57.14% 2 Missing and 1 partial ⚠️
...ceselector/StrictReplicaGroupInstanceSelector.java 0.00% 2 Missing and 1 partial ⚠️
...roker/requesthandler/BaseBrokerRequestHandler.java 77.77% 2 Missing ⚠️
...a/org/apache/pinot/core/transport/QueryRouter.java 60.00% 1 Missing and 1 partial ⚠️
... and 7 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #11978      +/-   ##
============================================
+ Coverage     61.52%   61.61%   +0.08%     
+ Complexity     1152      207     -945     
============================================
  Files          2386     2390       +4     
  Lines        129565   129823     +258     
  Branches      20053    20082      +29     
============================================
+ Hits          79721    79990     +269     
+ Misses        44026    44001      -25     
- Partials       5818     5832      +14     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (?)
integration <0.01% <0.00%> (-0.01%) ⬇️
integration1 <0.01% <0.00%> (-0.01%) ⬇️
integration2 0.00% <0.00%> (ø)
java-11 61.56% <43.33%> (?)
java-21 61.48% <43.33%> (-0.05%) ⬇️
skip-bytebuffers-false 61.58% <43.33%> (+0.08%) ⬆️
skip-bytebuffers-true 61.46% <43.33%> (-0.03%) ⬇️
temurin 61.61% <43.33%> (+0.08%) ⬆️
unittests 61.61% <43.33%> (+0.08%) ⬆️
unittests1 46.87% <44.82%> (+0.09%) ⬆️
unittests2 27.62% <32.50%> (+0.01%) ⬆️

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.

@klsince klsince force-pushed the allow_optional_segments branch 2 times, most recently from 289ac57 to b7f7e7b Compare November 13, 2023 21:49
@klsince klsince force-pushed the allow_optional_segments branch from b7f7e7b to 1f46ec9 Compare November 14, 2023 21:35
@klsince klsince changed the title [Draft] Allow optional segments Allow optional segments that can be skipped by servers without failing the query Nov 14, 2023
@klsince klsince force-pushed the allow_optional_segments branch from a9e7bce to 39342e2 Compare November 14, 2023 23:10
@klsince klsince requested a review from Jackie-Jiang November 15, 2023 18:52
@klsince klsince force-pushed the allow_optional_segments branch 2 times, most recently from a9c0462 to a716986 Compare November 16, 2023 22:37
@klsince klsince requested a review from Jackie-Jiang November 16, 2023 23:21
@klsince klsince force-pushed the allow_optional_segments branch 3 times, most recently from 90e5feb to 1e2d52f Compare November 20, 2023 22:17
@klsince klsince requested a review from Jackie-Jiang November 21, 2023 06:51
jhyao pushed a commit to jhyao/pinot that referenced this pull request Nov 21, 2023
@klsince klsince force-pushed the allow_optional_segments branch from c5cb275 to ba961e5 Compare November 27, 2023 20:32
@klsince klsince requested a review from Jackie-Jiang November 27, 2023 23:48
@klsince klsince merged commit 3bd74b3 into apache:master Nov 29, 2023
@klsince klsince deleted the allow_optional_segments branch November 29, 2023 01:00
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.

3 participants