Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support reloading consuming segment using force commit #9640

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Oct 22, 2022

Another try of #9310

  • Use force commit to achieve consuming segment reload
  • Modify HybridClusterIntegrationTest to use LLC to test the consuming segment reload
  • Make LLC default for integration test because most (if not all) use cases use LLC
  • Also fix a bug for adding empty segment to dedup table

Copy link
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.

LGTM

@Jackie-Jiang Jackie-Jiang force-pushed the reload_consuming_segment branch 2 times, most recently from baad5f2 to 07a6ad7 Compare October 24, 2022 17:36
@jugomezv
Copy link
Contributor

Another try of #9310

  • Use force commit to achieve consuming segment reload
  • Modify HybridClusterIntegrationTest to use LLC to test the consuming segment reload
  • Make LLC default for integration test because most (if not all) use cases use LLC

@Jackie-Jiang I will go ahead and delete the PR I started as you pushed the change here. Thanks for driving it as I have been real busy at work these days

@Jackie-Jiang
Copy link
Contributor Author

@jugomezv Thanks for trying it out. I didn't realize it requires changing the integration test in the beginning

@Jackie-Jiang Jackie-Jiang force-pushed the reload_consuming_segment branch from 07a6ad7 to 6bbce06 Compare October 25, 2022 06:46
@Jackie-Jiang Jackie-Jiang force-pushed the reload_consuming_segment branch from 6bbce06 to 7fa61a3 Compare October 25, 2022 17:42
@codecov-commenter
Copy link

Codecov Report

Merging #9640 (7fa61a3) into master (77fa36d) will increase coverage by 40.49%.
The diff coverage is 60.86%.

@@              Coverage Diff              @@
##             master    #9640       +/-   ##
=============================================
+ Coverage     28.09%   68.58%   +40.49%     
- Complexity       53     5308     +5255     
=============================================
  Files          1935     1947       +12     
  Lines        103815   104145      +330     
  Branches      15758    15790       +32     
=============================================
+ Hits          29162    71431    +42269     
+ Misses        71780    27664    -44116     
- Partials       2873     5050     +2177     
Flag Coverage Δ
integration1 ?
integration2 24.82% <26.08%> (+0.34%) ⬆️
unittests1 67.36% <80.00%> (?)
unittests2 15.67% <0.00%> (?)

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

Impacted Files Coverage Δ
...server/starter/helix/HelixInstanceDataManager.java 75.57% <38.46%> (+0.23%) ⬆️
...local/indexsegment/mutable/MutableSegmentImpl.java 57.88% <85.71%> (+57.88%) ⬆️
...ata/manager/realtime/RealtimeTableDataManager.java 68.96% <100.00%> (+1.53%) ⬆️
...segment/local/data/manager/SegmentDataManager.java 78.57% <100.00%> (+78.57%) ⬆️
...nt/virtualcolumn/VirtualColumnProviderFactory.java 58.33% <100.00%> (+58.33%) ⬆️
...pinot/minion/exception/TaskCancelledException.java 0.00% <0.00%> (-100.00%) ⬇️
...nverttorawindex/ConvertToRawIndexTaskExecutor.java 0.00% <0.00%> (-100.00%) ⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java 0.00% <0.00%> (-94.74%) ⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java 0.00% <0.00%> (-87.10%) ⬇️
.../transform/function/MapValueTransformFunction.java 0.00% <0.00%> (-85.30%) ⬇️
... and 1392 more

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

@Jackie-Jiang Jackie-Jiang force-pushed the reload_consuming_segment branch from 7fa61a3 to e49842b Compare October 25, 2022 19:21
}
LOGGER.info("Reloading (force committing) LLC consuming segment: {} in table: {}", segmentName,
tableNameWithType);
((LLRealtimeSegmentDataManager) segmentDataManager).forceCommit();
Copy link
Contributor

@siddharthteotia siddharthteotia Oct 27, 2022

Choose a reason for hiding this comment

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

So forceCommit is async ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is async. We can only signal the segment to perform the commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants