Skip to content

refactor LLRealtimeSegmentDataManager#9424

Merged
jackjlli merged 9 commits intoapache:masterfrom
navina:ll-consumer-fixes
Sep 26, 2022
Merged

refactor LLRealtimeSegmentDataManager#9424
jackjlli merged 9 commits intoapache:masterfrom
navina:ll-consumer-fixes

Conversation

@navina
Copy link
Copy Markdown
Contributor

@navina navina commented Sep 19, 2022

There is no change in functionality in this PR. The changes are:

  • Created a wrapper class to encapsulate all the column descriptors used in LLRealtimeSegmentDataManager
  • LLRealtimeSegmentDataManager should always use createPartitionMetadataProvider and not createStreamMetadataProvider

@navina
Copy link
Copy Markdown
Contributor Author

navina commented Sep 19, 2022

@Jackie-Jiang / @KKcorps / @npawar : please review!

@navina navina requested a review from KKcorps September 19, 2022 07:25
jackjlli
jackjlli reviewed Sep 19, 2022
View reviewed changes
.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java Outdated
Copy link
Copy Markdown
Member

@jackjlli jackjlli Sep 19, 2022

Choose a reason for hiding this comment

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

Should we consider renaming the caller method name?

Copy link
Copy Markdown
Contributor Author

@navina navina Sep 20, 2022

Choose a reason for hiding this comment

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

I don't know why an interface called StreamMetadataProvider contains a method for both createStreamMetadataProvider (stream-level) and createPartitionMetadataProvider (partition-level). I am sure there some history to do this.
what is your proposal here?

Copy link
Copy Markdown
Member

@jackjlli jackjlli Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know why an interface called StreamMetadataProvider contains a method for both createStreamMetadataProvider (stream-level) and createPartitionMetadataProvider (partition-level).

I think it's because the high-level segment data manager was implemented before the low-level segment data manager. But since the caller method is a private class, we can make it more intuitive to understand by just renaming it.

Copy link
Copy Markdown
Contributor Author

@navina navina Sep 21, 2022

Choose a reason for hiding this comment

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

Ok. I renamed the method to createPartitionMetadataProvider

Copy link
Copy Markdown
Member

@jackjlli jackjlli Sep 21, 2022

Choose a reason for hiding this comment

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

Sorry for the late comment here. Somehow I forgot to submit the review yesterday.

By taking a closer look, the purpose of _streamMetadataProvider is to fetch the latest value of partitionCount from the stream provider, which shouldn't associate with any particular partitionId here. We should stick to using the stream-level stream metadata provider here.

@navina navina requested review from KKcorps and jackjlli and removed request for KKcorps, jackjlli and sajjad-moradi September 20, 2022 05:10
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 20, 2022 •
edited
Loading

Codecov Report

Merging #9424 (e66033e) into master (165e26f) will increase coverage by 1.30%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #9424      +/-   ##
============================================
+ Coverage     68.59%   69.90%   +1.30%     
- Complexity     4710     4744      +34     
============================================
  Files          1904     1910       +6     
  Lines        101661   101758      +97     
  Branches      15433    15440       +7     
============================================
+ Hits          69735    71133    +1398     
+ Misses        26924    25609    -1315     
- Partials       5002     5016      +14
Flag Coverage Δ
integration1 26.04% <46.96%> (+0.08%) ⬆️
integration2 24.77% <50.00%> (?)
unittests1 67.12% <64.61%> (+0.02%) ⬆️
unittests2 15.51% <0.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
.../plugin/stream/kinesis/KinesisConsumerFactory.java 0.00% <0.00%> (ø)
...l/realtime/converter/RealtimeSegmentConverter.java 79.24% <69.23%> (-1.46%) ⬇️
...manager/realtime/LLRealtimeSegmentDataManager.java 70.22% <84.37%> (+2.05%) ⬆️
...ltime/converter/ColumnIndicesForRealtimeTable.java 92.85% <92.85%> (ø)
...manager/realtime/HLRealtimeSegmentDataManager.java 82.87% <100.00%> (+8.91%) ⬆️
...inion/event/DefaultMinionEventObserverFactory.java 50.00% <0.00%> (-16.67%) ⬇️
...nction/DistinctCountBitmapAggregationFunction.java 47.15% <0.00%> (-10.89%) ⬇️
...pinot/core/query/request/context/TimerContext.java 91.66% <0.00%> (-4.17%) ⬇️
...pache/pinot/minion/event/MinionEventObservers.java 89.65% <0.00%> (-3.45%) ⬇️
...core/query/pruner/SelectionQuerySegmentPruner.java 83.52% <0.00%> (-2.36%) ⬇️
... and 162 more

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

@navina navina requested review from jackjlli and removed request for KKcorps September 21, 2022 06:10
jackjlli
jackjlli approved these changes Sep 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jackjlli jackjlli left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing all the comments!

.../src/main/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManager.java
createPartitionMetadataProvider("Fetch latest stream offset");
}
try {
return _partitionMetadataProvider.fetchStreamPartitionOffset(OffsetCriteria.LARGEST_OFFSET_CRITERIA,
Copy link
Copy Markdown
Member

@jackjlli jackjlli Sep 26, 2022

Choose a reason for hiding this comment

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

+1 on this. No need to instantiate another partitionLevelMetadataProvider here.

@jackjlli jackjlli merged commit 4d79954 into apache:master Sep 26, 2022
61yao pushed a commit to 61yao/pinot that referenced this pull request Oct 3, 2022
@navina
refactor LLRealtimeSegmentDataManager (apache#9424)
cad25a9 
* use only createPartitionMetadataProvider in LLRealtimeSegmentDataManager

* Moving columns into a container class

* remove from HLRealtimeSegmentDataManager

* Addressing PR feedback

* Remove instance variables

* rename private method

* renaming stream with partition

* lint

* kinsis consumer factory should return streamMetadataProvider for both partition and stream level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@npawar npawar npawar left review comments

@KKcorps KKcorps KKcorps approved these changes

@sajjad-moradi sajjad-moradi sajjad-moradi approved these changes

@jackjlli jackjlli jackjlli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@navina @codecov-commenter @KKcorps @sajjad-moradi @npawar @jackjlli