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

Always pass clientId to kafka's consumer properties #9444

Merged
merged 1 commit into from Sep 21, 2022

Conversation

navina
Copy link
Contributor

@navina navina commented Sep 21, 2022

kafka consumer plugin doesn't pass the provided clientId into the underlying consumer properties. This leads to kafka mbeans that look like consumer-null-87 .
Instead, the provided clientId should always be set as a kafka consumer property.

@navina
Copy link
Contributor Author

navina commented Sep 21, 2022

@KKcorps / @saurabhd336 / @npawar : please review! and maybe, add tags as bug

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2022

Codecov Report

Merging #9444 (091b429) into master (b6f3331) will increase coverage by 5.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #9444      +/-   ##
============================================
+ Coverage     63.57%   68.59%   +5.02%     
+ Complexity     4766     4704      -62     
============================================
  Files          1837     1890      +53     
  Lines         98649   100960    +2311     
  Branches      15086    15353     +267     
============================================
+ Hits          62712    69255    +6543     
+ Misses        31333    26749    -4584     
- Partials       4604     4956     +352     
Flag Coverage Δ
integration1 26.03% <100.00%> (?)
unittests1 67.11% <ø> (-0.01%) ⬇️
unittests2 15.34% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...lugin/stream/kafka20/KafkaStreamLevelConsumer.java 88.33% <ø> (+88.33%) ⬆️
.../kafka20/KafkaPartitionLevelConnectionHandler.java 90.47% <100.00%> (+10.47%) ⬆️
.../helix/core/minion/MinionInstancesCleanupTask.java 77.27% <0.00%> (-4.55%) ⬇️
...gregation/function/StUnionAggregationFunction.java 73.52% <0.00%> (-2.95%) ⬇️
...ders/forward/VarByteChunkMVForwardIndexReader.java 77.57% <0.00%> (-1.87%) ⬇️
.../aggregation/function/ModeAggregationFunction.java 87.83% <0.00%> (-0.55%) ⬇️
...ter/helix/OffsetBasedConsumptionStatusChecker.java 0.00% <0.00%> (ø)
...gin/inputformat/avro/SimpleAvroMessageDecoder.java 0.00% <0.00%> (ø)
.../helix/FreshnessBasedConsumptionStatusChecker.java 0.00% <0.00%> (ø)
.../apache/pinot/server/worker/WorkerQueryServer.java 70.00% <0.00%> (ø)
... and 365 more

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

Copy link
Contributor

@KKcorps KKcorps left a comment

Choose a reason for hiding this comment

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

One minor comment. Otherwise, LGTM!

@KKcorps KKcorps added the bugfix label Sep 21, 2022
@npawar npawar merged commit b4b0a94 into apache:master Sep 21, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants