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.

Sign up for GitHub

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

Jump to bottom

Improve Adaptive Server Selection to penalize servers returning server side exceptions #14029

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Improve Adaptive Server Selection to penalize servers returning server side exceptions #14029

merged 4 commits into from
Sep 30, 2024

Conversation

kiruphabalu
Copy link
Contributor

@kiruphabalu kiruphabalu commented Sep 18, 2024 •
edited by vvivekiyer
Loading

  • Currently if server response has exceptions they are not considered and the latency is set to time taken to receive response. This potentially ranks servers with underlying issues better. So the algorithm would prefer these servers.
  • With this change, the response from server is parsed and if there are server side exceptions the latency of the server is set to max as timeout ms. This will rank the server lower than healthy servers.

Unit test cases

  • Response with no exception will have latency set to response time
  • Response with client exceptions will have latency set to response time
  • Response with at least one server exception will have latency set to timeout ms

@kiruphabalu
PINOT-19249 ADSS penalize server with hardware issues WIP push
228d6da 
PINOT-19249 Adding more unit test cases

PINOT-19249 Addressing comments and changing code to cover edge cases

PINOT-19249 Code changes based on comments

PINOT-19249 Fixing test cases for ADSS penalizing servers with exceptions
@vvivekiyer vvivekiyer changed the title Rank servers with exceptions lower in ADSS Improve Adaptive Server Selection to penalize servers returning server side exceptions Sep 18, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.00%. Comparing base (59551e4) to head (f34b78b).
Report is 1097 commits behind head on master.

Files with missing lines Patch % Lines
...pache/pinot/core/transport/AsyncQueryResponse.java 84.61% 0 Missing and 2 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #14029      +/-   ##
============================================
+ Coverage     61.75%   64.00%   +2.25%     
- Complexity      207     1538    +1331     
============================================
  Files          2436     2596     +160     
  Lines        133233   143356   +10123     
  Branches      20636    21958    +1322     
============================================
+ Hits          82274    91757    +9483     
+ Misses        44911    44836      -75     
- Partials       6048     6763     +715
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.97% <84.61%> (+2.26%) ⬆️
java-21 63.88% <84.61%> (+2.25%) ⬆️
skip-bytebuffers-false 64.00% <84.61%> (+2.25%) ⬆️
skip-bytebuffers-true 63.85% <84.61%> (+36.12%) ⬆️
temurin 64.00% <84.61%> (+2.25%) ⬆️
unittests 64.00% <84.61%> (+2.25%) ⬆️
unittests1 55.71% <84.61%> (+8.82%) ⬆️
unittests2 34.46% <0.00%> (+6.73%) ⬆️

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.

@kiruphabalu kiruphabalu marked this pull request as ready for review September 19, 2024 22:35
jasperjiaguo
jasperjiaguo reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@jasperjiaguo jasperjiaguo left a comment

Choose a reason for hiding this comment

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

lgtm mostly could we add some more comments?

pinot-core/src/main/java/org/apache/pinot/core/transport/AsyncQueryResponse.java Show resolved Hide resolved
pinot-core/src/test/java/org/apache/pinot/core/transport/QueryRoutingTest.java Outdated Show resolved Hide resolved
jasperjiaguo
jasperjiaguo approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@jasperjiaguo jasperjiaguo left a comment

Choose a reason for hiding this comment

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

lgtm

vvivekiyer
vvivekiyer reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@vvivekiyer vvivekiyer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @kiruphabalu

@vvivekiyer vvivekiyer merged commit 3452ef9 into apache:master Sep 30, 2024
21 checks passed
@itschrispeck itschrispeck mentioned this pull request Oct 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vvivekiyer vvivekiyer vvivekiyer left review comments

@jasperjiaguo jasperjiaguo jasperjiaguo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kiruphabalu @codecov-commenter @jasperjiaguo @vvivekiyer