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

Adding Pinot Minion client #6339

Merged
merged 2 commits into from
Dec 9, 2020
Merged

Adding Pinot Minion client #6339

merged 2 commits into from
Dec 9, 2020

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Dec 9, 2020

Description

Adding Pinot Minion client to operate on controller /tasks APIs.

@codecov-io
Copy link

codecov-io commented Dec 9, 2020

Codecov Report

Merging #6339 (dabca09) into master (1beaab5) will decrease coverage by 20.90%.
The diff coverage is 51.88%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #6339       +/-   ##
===========================================
- Coverage   66.44%   45.53%   -20.91%     
===========================================
  Files        1075     1285      +210     
  Lines       54773    62034     +7261     
  Branches     8168     9001      +833     
===========================================
- Hits        36396    28250     -8146     
- Misses      15700    31475    +15775     
+ Partials     2677     2309      -368     
Flag Coverage Δ
integration 45.53% <51.88%> (?)

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

Impacted Files Coverage Δ
...ot/broker/broker/AllowAllAccessControlFactory.java 100.00% <ø> (ø)
.../helix/BrokerUserDefinedMessageHandlerFactory.java 52.83% <0.00%> (-13.84%) ⬇️
...org/apache/pinot/broker/queryquota/HitCounter.java 0.00% <0.00%> (-100.00%) ⬇️
...che/pinot/broker/queryquota/MaxHitRateTracker.java 0.00% <0.00%> (ø)
...ache/pinot/broker/queryquota/QueryQuotaEntity.java 0.00% <0.00%> (-50.00%) ⬇️
...ker/routing/instanceselector/InstanceSelector.java 100.00% <ø> (ø)
...ceselector/StrictReplicaGroupInstanceSelector.java 0.00% <0.00%> (ø)
.../main/java/org/apache/pinot/client/Connection.java 22.22% <0.00%> (-26.62%) ⬇️
...not/common/assignment/InstancePartitionsUtils.java 64.28% <ø> (-8.89%) ⬇️
.../apache/pinot/common/exception/QueryException.java 90.27% <ø> (+5.55%) ⬆️
... and 1281 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed9f122...dabca09. Read the comment docs.

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 otherwise.
Minion is not ready to be used out of the box, and the APIs are subject to be changed. Not sure if we should add the client now, or hold until minion is ready

import org.testng.annotations.Test;


public class MinionClientTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not actually testing the functionality as the response is hard-coded within the test. You may modify the minion integration test to test against the controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@xiangfu0
Copy link
Contributor Author

xiangfu0 commented Dec 9, 2020

LGTM otherwise.
Minion is not ready to be used out of the box, and the APIs are subject to be changed. Not sure if we should add the client now, or hold until minion is ready

this is a good question, I will comment and make it a test feature I guess. I feel it would be good to have it and it's ok to change it and deprecate the methods in the future.

@xiangfu0 xiangfu0 merged commit c124334 into master Dec 9, 2020
@xiangfu0 xiangfu0 deleted the adding-minion-clients branch December 9, 2020 05:35
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