Skip to content

Make realtime indexes pluggable#8279

Merged
richardstartin merged 3 commits intoapache:masterfrom
richardstartin:realtime-index-overrides
Mar 7, 2022
Merged

Make realtime indexes pluggable#8279
richardstartin merged 3 commits intoapache:masterfrom
richardstartin:realtime-index-overrides

Conversation

@richardstartin
Copy link
Copy Markdown
Member

@richardstartin richardstartin commented Mar 2, 2022

This is PR simply moves static realtime index provisioning logic out of MutableSegmentImpl's constructor and into a default implementation of an SPI which can overridden. This PR follows an identical approach to making offline indexes pluggable, the rationale and approach were described in #7895.

The changes depend on a of package move (#8278) which I would like to get merged in before rebasing this PR to reduce the surface area of this one.

@richardstartin richardstartin force-pushed the realtime-index-overrides branch 2 times, most recently from d678896 to e7dfaff Compare March 2, 2022 18:36
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 2, 2022

Codecov Report

❌ Patch coverage is 0% with 137 lines in your changes missing coverage. Please review.
✅ Project coverage is 14.14%. Comparing base (e498a71) to head (b8c860b).
⚠️ Report is 6316 commits behind head on master.

Files with missing lines Patch % Lines
...pi/index/mutable/provider/MutableIndexContext.java 0.00% 62 Missing ⚠️
...exsegment/mutable/DefaultMutableIndexProvider.java 0.00% 35 Missing ⚠️
...local/indexsegment/mutable/MutableSegmentImpl.java 0.00% 17 Missing ⚠️
...che/pinot/segment/spi/index/IndexingOverrides.java 0.00% 17 Missing ⚠️
...dindex/RealtimeLuceneIndexReaderRefreshThread.java 0.00% 2 Missing ⚠️
...me/impl/invertedindex/RealtimeLuceneTextIndex.java 0.00% 2 Missing ⚠️
...time/impl/invertedindex/RealtimeInvertedIndex.java 0.00% 1 Missing ⚠️
...local/realtime/impl/json/MutableJsonIndexImpl.java 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (e498a71) and HEAD (b8c860b). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (e498a71) HEAD (b8c860b)
unittests1 1 0
integration1 1 0
integration2 1 0
Additional details and impacted files
@@              Coverage Diff              @@
##             master    #8279       +/-   ##
=============================================
- Coverage     70.82%   14.14%   -56.69%     
+ Complexity     4239       81     -4158     
=============================================
  Files          1631     1588       -43     
  Lines         85461    83678     -1783     
  Branches      12877    12677      -200     
=============================================
- Hits          60527    11833    -48694     
- Misses        20749    70961    +50212     
+ Partials       4185      884     -3301     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 ?
unittests2 14.14% <0.00%> (-0.03%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@richardstartin richardstartin force-pushed the realtime-index-overrides branch from e7dfaff to b839e30 Compare March 2, 2022 21:33
@richardstartin richardstartin force-pushed the realtime-index-overrides branch from b839e30 to f07e794 Compare March 2, 2022 22:20
Copy link
Copy Markdown
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.

LGTM otherwise

Copy link
Copy Markdown
Contributor

@sajjad-moradi sajjad-moradi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left a few minor comments.

@kishoreg
Copy link
Copy Markdown
Member

kishoreg commented Mar 4, 2022

@sajjad-moradi We rarely block a PR unless the code design is completely broken. I looked at the comments and I see that you have mentioned that most of them are subjective.

Can you please unblock us if what you are requesting is subjective. Thanks for understanding.

@richardstartin richardstartin force-pushed the realtime-index-overrides branch from 8c615ff to b8c860b Compare March 4, 2022 18:39
@sajjad-moradi
Copy link
Copy Markdown
Contributor

@sajjad-moradi We rarely block a PR unless the code design is completely broken. I looked at the comments and I see that you have mentioned that most of them are subjective.

Can you please unblock us if what you are requesting is subjective. Thanks for understanding.

Of course. Didn't know about this convention. The PR really looks good in general.

@richardstartin
Copy link
Copy Markdown
Member Author

@sajjad-moradi We rarely block a PR unless the code design is completely broken. I looked at the comments and I see that you have mentioned that most of them are subjective.
Can you please unblock us if what you are requesting is subjective. Thanks for understanding.

Of course. Didn't know about this convention. The PR really looks good in general.

Thanks, I felt it was important to run the changes past you or @mcvsubbu as a courtesy, because you might need to debug something introduced here, but when it comes to naming (or composition vs inheritance) there tend to be as many opinions as there are people.

@richardstartin richardstartin merged commit d3fe951 into apache:master Mar 7, 2022
@richardstartin richardstartin added the extension-point Adds or modifies an extension/SPI point label Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extension-point Adds or modifies an extension/SPI point

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants