Codecov Report

Merging #9624 (7609a9c) into master (77fa36d) will decrease coverage by 2.19%.
The diff coverage is 16.66%.

@@             Coverage Diff              @@
##             master    #9624      +/-   ##
============================================
- Coverage     28.09%   25.89%   -2.20%     
+ Complexity       53       44       -9     
============================================
  Files          1935     1934       -1     
  Lines        103815   103917     +102     
  Branches      15758    15770      +12     
============================================
- Hits          29162    26913    -2249     
- Misses        71780    74297    +2517     
+ Partials       2873     2707     -166     

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

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

High level question: why is the logic implemented in the table rebalancer? IMO it should be part of the tier assigner

The design decision was made to extend SegmentRelocator (not Assigner either to separate responsibilities). I extended TableRebalancer (used by SegmentRelocator) to check on a condition i.e.

if (currentAssignment.equals(targetAssignment)) {
      LOGGER.info("Table: {} is already balanced", tableNameWithType);
...

But looks like I can check on RebalanceResult.Status.DONE anyway, to move tier migration logic into SegmentRelocator to be clear.

But why do we want to use a separate periodic task to trigger the reload? We should just trigger the reload in SegmentTierAssigner which modify the tier for the segment. Once the SegmentTierAssigner changes the target tier for a segment, the new loaded segment will already be moved, and we want the already loaded segment to have the same behavior to be consistent.

A new target tier can lead to two reactions:

1. segment needs to go to a new server (when the target tier is only defined for a specific pool of servers as identified via serverTag in the tierConfig);
2. segment is already on the right server, but still needs to go to a new tier, backed by a different datadir.

Action 1 is handled by TableRebalancer; Action 2 is handled by SegmentReloadMessage. Extending SegmentRelocator, it's easy to control the sequence of those two actions, to do Action 2 after Action 1 is completed. So that, we don't send SegmentReloadMessage to a server that's not the ideal server for the segment yet, and it reloads the segment unnecessarily.

It is okay to do it within the SegmentRelocator, but we should also move the logic in SegmentTierAssigner into it. We don't want to use 2 periodic tasks to perform the same operation. Basically after assigning the tier, we should trigger the reload immediately, instead of relying on another periodic task to trigger it.

... Basically after assigning the tier, we should trigger the reload immediately, instead of relying on another periodic task to trigger it.

I think there may be use cases to separate the two: 1) keep SegmentTierAssigner running to calculate target tier continuously; 2) but not run SegmentRelocator task, instead do table rebalance and tier migration manually as planned maintenance based on prod env, like when cluster does not get queries.