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

Fix NPE when reading ZK address from controller config #9751

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

Jackie-Jiang
Copy link
Contributor

Fix the NPE in #9745

@61yao
Copy link
Contributor

61yao commented Nov 8, 2022

Nit: are we able to cover this in unit test if possible?

@Jackie-Jiang
Copy link
Contributor Author

Nit: are we able to cover this in unit test if possible?

The change in this PR basically change the NPE to a more readable error message, and IMO it is a little bit overkilling to add a test for it

@Jackie-Jiang Jackie-Jiang force-pushed the fix_controller_conf_zk_str branch from 975157f to 58aaaba Compare November 19, 2022 11:55
@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2022

Codecov Report

Merging #9751 (58aaaba) into master (3724ba2) will increase coverage by 35.65%.
The diff coverage is 66.66%.

@@              Coverage Diff              @@
##             master    #9751       +/-   ##
=============================================
+ Coverage     34.69%   70.35%   +35.65%     
- Complexity      190     5001     +4811     
=============================================
  Files          1965     1965               
  Lines        105115   105108        -7     
  Branches      15909    15908        -1     
=============================================
+ Hits          36474    73950    +37476     
+ Misses        65542    26017    -39525     
- Partials       3099     5141     +2042     
Flag Coverage Δ
integration1 25.24% <66.66%> (+0.11%) ⬆️
integration2 24.49% <66.66%> (?)
unittests1 67.96% <ø> (?)
unittests2 15.83% <66.66%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...va/org/apache/pinot/controller/ControllerConf.java 59.68% <66.66%> (+2.70%) ⬆️
.../helix/core/realtime/SegmentCompletionManager.java 72.15% <0.00%> (-0.82%) ⬇️
...pache/pinot/core/query/utils/idset/EmptyIdSet.java 25.00% <0.00%> (ø)
...inot/core/util/SegmentCompletionProtocolUtils.java 57.69% <0.00%> (ø)
...anager/realtime/SegmentBuildTimeLeaseExtender.java 63.23% <0.00%> (ø)
.../core/realtime/PinotLLCRealtimeSegmentManager.java 72.07% <0.00%> (+0.45%) ⬆️
...che/pinot/broker/routing/BrokerRoutingManager.java 86.07% <0.00%> (+0.55%) ⬆️
...e/pinot/common/function/TransformFunctionType.java 100.00% <0.00%> (+0.94%) ⬆️
...x/core/realtime/MissingConsumingSegmentFinder.java 87.09% <0.00%> (+1.07%) ⬆️
.../apache/pinot/common/exception/QueryException.java 94.44% <0.00%> (+1.11%) ⬆️
... and 1118 more

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

@Jackie-Jiang Jackie-Jiang merged commit 591356a into apache:master Nov 21, 2022
@Jackie-Jiang Jackie-Jiang deleted the fix_controller_conf_zk_str branch November 21, 2022 18:10
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.

4 participants