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

getMessageAtIndex should actually return the value in the streamMessage for compatibility #11355

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

navina
Copy link
Contributor

@navina navina commented Aug 15, 2023

Fixing a bug from #10995

@navina
Copy link
Contributor Author

navina commented Aug 15, 2023

@KKcorps : review pls!

@Jackie-Jiang should I clean up this deprecated spi- method (MessageBatch#getMessageAtIndex) before the next release?

@codecov-commenter
Copy link

codecov-commenter commented Aug 15, 2023

Codecov Report

Merging #11355 (402a48f) into master (4c307e1) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master   #11355      +/-   ##
============================================
- Coverage     61.52%   61.52%   -0.01%     
+ Complexity     6514     6512       -2     
============================================
  Files          2233     2233              
  Lines        120083   120084       +1     
  Branches      18223    18223              
============================================
- Hits          73880    73877       -3     
+ Misses        40801    40800       -1     
- Partials       5402     5407       +5     
Flag Coverage Δ
integration1 0.00% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 61.47% <0.00%> (-0.01%) ⬇️
java-17 61.36% <0.00%> (+<0.01%) ⬆️
java-20 61.37% <0.00%> (-0.01%) ⬇️
temurin 61.52% <0.00%> (-0.01%) ⬇️
unittests1 66.98% <ø> (-0.03%) ⬇️
unittests2 14.65% <0.00%> (+0.01%) ⬆️

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

Files Changed Coverage Δ
...pinot/plugin/stream/pulsar/PulsarMessageBatch.java 25.92% <0.00%> (-1.00%) ⬇️

... and 10 files with indirect coverage changes

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

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.

The fix looks good to me.

Not introduced in this PR, but why do we prefer reading the bytes instead of directly reading the message?

@navina
Copy link
Contributor Author

navina commented Aug 16, 2023

Not introduced in this PR, but why do we prefer reading the bytes instead of directly reading the message?

The original change I introduced in #9224 was to cleanly decouple the interfaces for "fetching" a record from stream and "decoding" the payload. But that broke LinkedIn's build which was using a custom implementation of stream consumer and doesn't provide the capability to fetch raw bytes from kafka stream. So, it was fixed forward in #9544 .

@Jackie-Jiang Jackie-Jiang merged commit e4220ec into apache:master Aug 16, 2023
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