Skip to content

Add help methods to check if segment needs reprocessing#7925

Merged
Jackie-Jiang merged 3 commits intoapache:masterfrom
klsince:check_if_need_preprocess
Dec 22, 2021
Merged

Add help methods to check if segment needs reprocessing#7925
Jackie-Jiang merged 3 commits intoapache:masterfrom
klsince:check_if_need_preprocess

Conversation

@klsince
Copy link
Contributor

@klsince klsince commented Dec 17, 2021

Description

Add helper methods to check if a segment needs reprocessing by comparing its metadata with the table config and schema.

Right now, the checking and reprocessing logic are mixed together in ImmutableSegmentLoader.load() method. The helper methods added here will allow us to check consistency before conducting any real reprocessing that can be heavy. For example, when a segment doesn't need reprocessing, we can save all the upfront cost of preparing the data for reprocessing, like downloading some data from remote tier backend.

This PR hasn't changed the existing control flow yet. The next one will.

Upgrade Notes

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

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

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

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

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

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

@klsince
Copy link
Contributor Author

klsince commented Dec 17, 2021

Found I was not able to reopen the previously closed PR: #7898 after rebasing new master. So I opened this PR, which doesn't add new changes but address comments from @Jackie-Jiang @siddharthteotia:
mainly about 1) add javadocs; 2) refine tests; 3) refactor the IndexHandler interface.

Sorry about the inconvenience. Please continue the review here.

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2021

Codecov Report

Merging #7925 (aaafdb5) into master (6037cac) will decrease coverage by 43.47%.
The diff coverage is 8.97%.

❗ Current head aaafdb5 differs from pull request most recent head 21debfe. Consider uploading reports for the commit 21debfe to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             master    #7925       +/-   ##
=============================================
- Coverage     71.15%   27.67%   -43.48%     
=============================================
  Files          1593     1584        -9     
  Lines         82365    82109      -256     
  Branches      12270    12260       -10     
=============================================
- Hits          58609    22726    -35883     
- Misses        19806    57320    +37514     
+ Partials       3950     2063     -1887     
Flag Coverage Δ
integration1 ?
integration2 27.67% <8.97%> (+0.08%) ⬆️
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
...core/query/executor/ServerQueryExecutorV1Impl.java 80.00% <0.00%> (-7.75%) ⬇️
...in/stream/kinesis/KinesisPartitionGroupOffset.java 0.00% <ø> (-17.40%) ⬇️
...not/plugin/stream/kinesis/KinesisRecordsBatch.java 0.00% <ø> (-57.15%) ⬇️
...indexsegment/immutable/ImmutableSegmentLoader.java 0.00% <0.00%> (-91.03%) ⬇️
.../io/writer/impl/BaseChunkSVForwardIndexWriter.java 0.00% <0.00%> (-85.00%) ⬇️
...riter/impl/VarByteChunkSVForwardIndexWriterV4.java 0.00% <0.00%> (-96.34%) ⬇️
...ocal/recordtransformer/ComplexTypeTransformer.java 0.00% <ø> (-55.68%) ⬇️
...ocal/segment/index/loader/IndexHandlerFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...ocal/segment/index/loader/SegmentPreProcessor.java 0.00% <0.00%> (-61.02%) ⬇️
...t/index/loader/bloomfilter/BloomFilterHandler.java 0.00% <0.00%> (-78.34%) ⬇️
... and 1164 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 6037cac...21debfe. Read the comment docs.

@klsince klsince force-pushed the check_if_need_preprocess branch from 26e58b3 to 9b814b2 Compare December 17, 2021 23:53
@klsince klsince force-pushed the check_if_need_preprocess branch from 9b814b2 to f83f88f Compare December 20, 2021 17:37
@klsince klsince force-pushed the check_if_need_preprocess branch from 4f12f94 to 21debfe Compare December 22, 2021 00:47
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.

4 participants