Skip to content

Use streamType for parsing and validating protobuf decoder config#11544

Closed
navina wants to merge 1 commit intoapache:masterfrom
navina:proto-bug-gh-11530
Closed

Use streamType for parsing and validating protobuf decoder config#11544
navina wants to merge 1 commit intoapache:masterfrom
navina:proto-bug-gh-11530

Conversation

@navina
Copy link
Contributor

@navina navina commented Sep 7, 2023

What does this PR do

Fixes #11530

@navina
Copy link
Contributor Author

navina commented Sep 7, 2023

Label: bugfix

@xiangfu0 / @Jackie-Jiang : review pls!

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2023

Codecov Report

Merging #11544 (8b60f67) into master (017e813) will decrease coverage by 0.04%.
Report is 1 commits behind head on master.
The diff coverage is 42.85%.

@@             Coverage Diff              @@
##             master   #11544      +/-   ##
============================================
- Coverage     63.06%   63.03%   -0.04%     
- Complexity     1106     1108       +2     
============================================
  Files          2320     2320              
  Lines        124664   124666       +2     
  Branches      19033    19033              
============================================
- Hits          78618    78580      -38     
- Misses        40441    40492      +51     
+ Partials       5605     5594      -11     
Flag Coverage Δ
integration <0.01% <0.00%> (ø)
integration1 <0.01% <0.00%> (ø)
integration2 0.00% <0.00%> (ø)
java-11 62.99% <42.85%> (-0.02%) ⬇️
java-17 62.89% <42.85%> (+0.01%) ⬆️
java-20 62.89% <42.85%> (-0.01%) ⬇️
temurin 63.03% <42.85%> (-0.04%) ⬇️
unittests 63.02% <42.85%> (-0.04%) ⬇️
unittests1 67.44% <42.85%> (-0.03%) ⬇️
unittests2 14.49% <0.00%> (-0.02%) ⬇️

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

Files Changed Coverage Δ
...he/pinot/segment/local/utils/TableConfigUtils.java 70.63% <42.85%> (-0.07%) ⬇️

... and 14 files with indirect coverage changes

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

📢 Have feedback on the report? Share it here.

@JeffBolle
Copy link
Contributor

When calling streamConfig.getDecoderProperties(), you get back a map of just the decoder properties that, for the test data, looks like this:

{protoClassName=test, descriptorFile=file://test}

The checks in validateDecoder can be substantially more simple (and not require any stream config knowledge).

I created a PR with my fixes here: #11551

Sorry @navina for putting us down the wrong path with too cursory of a look at the validation code.

@xiangfu0
Copy link
Contributor

Closed as a duplicate.

@xiangfu0 xiangfu0 closed this Sep 10, 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.

Protobuf Stream Config validationg assumes Kafka streams

4 participants