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

Adaptive Server Selection: Address pending review comments #9462

Merged
merged 2 commits into from
Sep 26, 2022

Conversation

vvivekiyer
Copy link
Contributor

@vvivekiyer vvivekiyer commented Sep 25, 2022

This PR addresses comments received in #9311 that could not be addressed before the PR got checked in

@vvivekiyer vvivekiyer force-pushed the serverSelectionReviewComments branch from 124ac9f to 83a8a29 Compare September 25, 2022 07:00
@vvivekiyer
Copy link
Contributor Author

@Jackie-Jiang please review.

@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2022

Codecov Report

Merging #9462 (83a8a29) into master (e2d8aaa) will decrease coverage by 1.21%.
The diff coverage is 54.54%.

@@             Coverage Diff              @@
##             master    #9462      +/-   ##
============================================
- Coverage     69.83%   68.62%   -1.22%     
+ Complexity     5127     4805     -322     
============================================
  Files          1909     1909              
  Lines        101749   101749              
  Branches      15440    15439       -1     
============================================
- Hits          71059    69823    -1236     
- Misses        25674    26934    +1260     
+ Partials       5016     4992      -24     
Flag Coverage Δ
integration1 26.06% <45.45%> (+0.10%) ⬆️
integration2 ?
unittests1 67.13% <100.00%> (+0.01%) ⬆️
unittests2 15.51% <18.18%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...routing/instanceselector/BaseInstanceSelector.java 99.11% <ø> (ø)
...instanceselector/ReplicaGroupInstanceSelector.java 42.85% <0.00%> (-2.05%) ⬇️
...org/apache/pinot/core/transport/QueryResponse.java 100.00% <ø> (ø)
...ing/instanceselector/BalancedInstanceSelector.java 84.61% <33.33%> (ø)
...ntroller/helix/core/PinotHelixResourceManager.java 69.76% <100.00%> (-1.85%) ⬇️
...pache/pinot/core/transport/AsyncQueryResponse.java 92.00% <100.00%> (ø)
...a/org/apache/pinot/core/transport/QueryRouter.java 80.43% <100.00%> (-3.27%) ⬇️
...t/core/plan/StreamingInstanceResponsePlanNode.java 0.00% <0.00%> (-100.00%) ⬇️
...ore/operator/streaming/StreamingResponseUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...server/starter/helix/SegmentReloadStatusValue.java 0.00% <0.00%> (-100.00%) ⬇️
... and 144 more

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

@vvivekiyer vvivekiyer marked this pull request as ready for review September 26, 2022 15:53
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. Thanks for addressing them

@Jackie-Jiang Jackie-Jiang merged commit 7b4ef53 into apache:master Sep 26, 2022
@siddharthteotia siddharthteotia changed the title Adaptive Server Selection: address review comments Adaptive Server Selection: Address pending review comments Sep 26, 2022
@vvivekiyer vvivekiyer deleted the serverSelectionReviewComments branch October 2, 2022 07:07
61yao pushed a commit to 61yao/pinot that referenced this pull request Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants