pinot icon indicating copy to clipboard operation
pinot copied to clipboard

introduce JsonUtils stringToMap

Open sullis opened this issue 1 year ago • 5 comments

sullis avatar Apr 02 '24 12:04 sullis

Codecov Report

Attention: Patch coverage is 35.71429% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 62.11%. Comparing base (59551e4) to head (5bc4376). Report is 244 commits behind head on master.

Files Patch % Lines
...a/org/apache/pinot/common/minion/MinionClient.java 35.71% 9 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #12776      +/-   ##
============================================
+ Coverage     61.75%   62.11%   +0.36%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2492      +56     
  Lines        133233   135952    +2719     
  Branches      20636    21036     +400     
============================================
+ Hits          82274    84443    +2169     
- Misses        44911    45262     +351     
- Partials       6048     6247     +199     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) :arrow_down:
integration <0.01% <0.00%> (-0.01%) :arrow_down:
integration1 <0.01% <0.00%> (-0.01%) :arrow_down:
integration2 0.00% <0.00%> (ø)
java-11 62.06% <35.71%> (+0.35%) :arrow_up:
java-21 61.96% <35.71%> (+0.33%) :arrow_up:
skip-bytebuffers-false 62.10% <35.71%> (+0.35%) :arrow_up:
skip-bytebuffers-true 61.91% <35.71%> (+34.18%) :arrow_up:
temurin 62.11% <35.71%> (+0.36%) :arrow_up:
unittests 62.10% <35.71%> (+0.36%) :arrow_up:
unittests1 46.97% <35.71%> (+0.08%) :arrow_up:
unittests2 27.75% <0.00%> (+0.02%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 02 '24 13:04 codecov-commenter

ready for review

sullis avatar Apr 02 '24 23:04 sullis

please review @gortiz @Jackie-Jiang

sullis avatar Apr 04 '24 17:04 sullis

Let's add a method Map<String, Object> stringToMap(String jsonString) into JsonUtils

would you prefer Map<String, Object> or Map<String, String>

MinionClient uses Map<String, String>

sullis avatar Apr 04 '24 21:04 sullis

Sorry didn't notice these 2 references are different.. In that case the previous commit is more clear

Jackie-Jiang avatar Apr 04 '24 22:04 Jackie-Jiang

Sorry didn't notice these 2 references are different.. In that case the previous commit is more clear

Ack. I have removed JsonUtils stringToMap. I just pushed the revert.

sullis avatar Apr 05 '24 14:04 sullis

Test suite passes

sullis avatar Apr 06 '24 01:04 sullis

Ready for review

sullis avatar Apr 08 '24 13:04 sullis