forceCommit mutable segements during segement reloads

[jugomezv]

Force-commit mutable segments on segment reloads. issue 9310

Testing: Check consuming segments transition to online upon schema changes when reload is requested

[codecov-commenter]

Codecov Report

Merging #9502 (11be977) into master (270315a) will decrease coverage by 6.28%. The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #9502      +/-   ##
============================================
- Coverage     69.90%   63.62%   -6.29%     
+ Complexity     4869     4846      -23     
============================================
  Files          1917     1864      -53     
  Lines        102206    99892    -2314     
  Branches      15510    15243     -267     
============================================
- Hits          71449    63558    -7891     
- Misses        25711    31663    +5952     
+ Partials       5046     4671     -375     

Flag	Coverage Δ
integration1	?
integration2	?
unittests1	67.18% <ø> (+0.01%)	:arrow_up:
unittests2	15.50% <ø> (-0.05%)	:arrow_down:

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

Impacted Files	Coverage Δ
...va/org/apache/pinot/core/routing/RoutingTable.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...va/org/apache/pinot/common/config/NettyConfig.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...a/org/apache/pinot/common/metrics/MinionMeter.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...g/apache/pinot/common/metrics/ControllerMeter.java	0.00% <0.00%> (-100.00%)	:arrow_down:
.../apache/pinot/common/metrics/BrokerQueryPhase.java	0.00% <0.00%> (-100.00%)	:arrow_down:
.../apache/pinot/common/metrics/MinionQueryPhase.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...ache/pinot/server/access/AccessControlFactory.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...he/pinot/common/messages/SegmentReloadMessage.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...he/pinot/common/messages/TableDeletionMessage.java	0.00% <0.00%> (-100.00%)	:arrow_down:
...pinot/core/data/manager/realtime/TimerService.java	0.00% <0.00%> (-100.00%)	:arrow_down:
... and 428 more

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

[jugomezv]

@Jackie-Jiang I am not sure this will solve my current issue which is that, the function where the force commit is placed, does not get invoked for the failing test but it does for other reload tests, but let me debug the failing test some more see why these reload code is not being called

[jugomezv]

@Jackie-Jiang I am not sure this will solve my current issue which is that, the function where the force commit is placed, does not get invoked for the failing test but it does for other reload tests, but let me debug the failing test some more see why these reload code is not being called

@Jackie-Jiang, @sajjad-moradi

I did rolled back this change and reran HybridClusteringIntegrationTest.testReload and found all segments of the realtime table are set to online--just before we run the reload test, but one of those segments still look mutable and code will try to reload it. Seems we do have a problem on the test or a bug with the part of code that sets the segments to consuming/online state. If you can take a look it is greatly appreciated. I will continue to go through the code see if I can figure it.

[sajjad-moradi]

@Jackie-Jiang I am not sure this will solve my current issue which is that, the function where the force commit is placed, does not get invoked for the failing test but it does for other reload tests, but let me debug the failing test some more see why these reload code is not being called

@Jackie-Jiang, @sajjad-moradi

I did rolled back this change and reran HybridClusteringIntegrationTest.testReload and found all segments of the realtime table are set to online--just before we run the reload test, but one of those segments still look mutable and code will try to reload it. Seems we do have a problem on the test or a bug with the part of code that sets the segments to consuming/online state. If you can take a look it is greatly appreciated. I will continue to go through the code see if I can figure it.

Looked into the failing test and looks like the realtime table used there is HighLevel, while the force commit only works for LowLevel mode!

@Jackie-Jiang Is there anyone using HighLevel consumption mode for realtime tables? If not, we should change these integration tests to LL in order to benefit from them.

[Jackie-Jiang]

I don't know anyone using HLC. Agree we should change hybrid table test to use LLC to get better coverage.

[jugomezv]

I don't know anyone using HLC. Agree we should change hybrid table test to use LLC to get better coverage.

OK I have been meaning to do this but have not got the time due to other things here at work, but I shall be able to test this soon

[Jackie-Jiang]

Any update on this? Also please take a look at the proposed code change, which should be more lightweight

[jugomezv]

being handled by this PR instead: PR9640