Skip to content

Update some usage of BatchConfig#7459

Merged
Jackie-Jiang merged 2 commits intoapache:masterfrom
walterddr:segment_writer_api_changes
Sep 21, 2021
Merged

Update some usage of BatchConfig#7459
Jackie-Jiang merged 2 commits intoapache:masterfrom
walterddr:segment_writer_api_changes

Conversation

@walterddr
Copy link
Contributor

Description

Looking at the usage of StreamConfig and BatchConfig. it seems like they should be used as

  • an internal representation after parsing the TableConfig class
  • performs validation to contents of the configuration instead of storing as plain Strings.

Therefore, changing some of the API usages of BatchConfig.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
NO

Does this PR fix a zero-downtime upgrade introduced earlier?
NO

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options - NO
  • Deprecation of configurations - NO
  • Signature changes to public methods/interfaces - YES
    • FileIngestionHelper no longer accepts BatchConfig as 3rd argument. instead a Map is required.
  • New plugins added or old plugins removed - NO

Documentation

N/A

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2021

Codecov Report

Merging #7459 (1fd9237) into master (fabda2b) will decrease coverage by 41.58%.
The diff coverage is 33.33%.

❗ Current head 1fd9237 differs from pull request most recent head 58b4f2b. Consider uploading reports for the commit 58b4f2b to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #7459       +/-   ##
=============================================
- Coverage     71.91%   30.33%   -41.59%     
=============================================
  Files          1519     1510        -9     
  Lines         75335    75002      -333     
  Branches      10980    10943       -37     
=============================================
- Hits          54177    22750    -31427     
- Misses        17508    50238    +32730     
+ Partials       3650     2014     -1636     
Flag Coverage Δ
integration1 30.33% <33.33%> (+0.01%) ⬆️
integration2 ?
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
...r/api/resources/PinotIngestionRestletResource.java 0.00% <0.00%> (ø)
...che/pinot/controller/util/FileIngestionHelper.java 0.00% <0.00%> (ø)
...ache/pinot/segment/local/utils/IngestionUtils.java 0.00% <0.00%> (-19.38%) ⬇️
...ot/spi/ingestion/segment/writer/SegmentWriter.java 0.00% <ø> (ø)
...org/apache/pinot/spi/plugin/PluginClassLoader.java 0.00% <0.00%> (-19.15%) ⬇️
...n/src/main/java/org/apache/pinot/common/Utils.java 60.46% <75.00%> (+21.33%) ⬆️
...ot/controller/api/access/AuthenticationFilter.java 62.85% <100.00%> (-5.72%) ⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java 87.09% <100.00%> (+1.91%) ⬆️
...egmentwriter/filebased/FileBasedSegmentWriter.java 85.71% <100.00%> (-6.94%) ⬇️
...c/main/java/org/apache/pinot/common/tier/Tier.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1048 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fabda2b...58b4f2b. Read the comment docs.

@Jackie-Jiang Jackie-Jiang merged commit 6976697 into apache:master Sep 21, 2021
walterddr pushed a commit to walterddr/pinot that referenced this pull request Sep 23, 2021
@walterddr walterddr mentioned this pull request Sep 23, 2021
apucher pushed a commit that referenced this pull request Sep 23, 2021
kriti-sc pushed a commit to kriti-sc/incubator-pinot that referenced this pull request Dec 12, 2021
kriti-sc pushed a commit to kriti-sc/incubator-pinot that referenced this pull request Dec 12, 2021
@walterddr walterddr deleted the segment_writer_api_changes branch December 6, 2023 16:24
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