Jackie-Jiang commented on code in PR #10118:
URL: https://github.com/apache/pinot/pull/10118#discussion_r1068744416
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/relocation/SegmentRelocator.java:
##########
@@ -130,40 +137,114 @@ protected void processTable(String tableNameWithType) {
_externalViewCheckIntervalInMs);
rebalanceConfig.addProperty(RebalanceConfigConstants.EXTERNAL_VIEW_STABILIZATION_TIMEOUT_IN_MS,
_externalViewStabilizationTimeoutInMs);
- // Run rebalance asynchronously
- _executorService.submit(() -> {
- try {
- // Relocating segments to new tiers needs two sequential actions:
table rebalance and local tier migration.
- // Table rebalance moves segments to the new ideal servers, which can
change for a segment when its target
- // tier is updated. New servers can put segments onto the right tier
when loading the segments. After that,
- // all segments are put on the right servers. If any segments are not
on their target tier, the server local
- // tier migration is triggered for them, basically asking the hosting
servers to reload them.
- //
- // We assume segment target tier is not changed between the two
actions, so update target tier here as well,
- // instead of using a separate task.
- // TODO: can add some sanity checks on the server side when reloading
segments to be more defensive, e.g. only
- // migrating segment to new tier when the hosting server is in
the server pool configured for that tier.
- updateTargetTier(tableNameWithType);
+ if (_rebalanceTablesSequentially) {
+ rebalanceTableSequentially(tableNameWithType, tableConfig,
rebalanceConfig);
+ } else {
+ _executorService.submit(() -> rebalanceTable(tableNameWithType,
tableConfig, rebalanceConfig));
+ }
+ }
- RebalanceResult rebalance =
- new
TableRebalancer(_pinotHelixResourceManager.getHelixZkManager()).rebalance(tableConfig,
rebalanceConfig);
- switch (rebalance.getStatus()) {
- case NO_OP:
- LOGGER.info("All segments are already relocated for table: {}",
tableNameWithType);
- migrateToTargetTier(tableNameWithType);
- break;
- case DONE:
- LOGGER.info("Finished relocating segments for table: {}",
tableNameWithType);
- migrateToTargetTier(tableNameWithType);
- break;
- default:
- LOGGER.error("Relocation failed for table: {}", tableNameWithType);
- break;
+ /**
+ * Use queue to rebalance tables sequentially, meanwhile make sure every
table gets its turn.
+ * @param tableNameWithType table requested to rebalance
+ */
+ private void rebalanceTableSequentially(String tableNameWithType,
TableConfig tableConfig,
Review Comment:
If I read correctly, if I call this method multiple times, all the tables
will be queued, but without one more call, only the first table will be
rebalanced.
Instead, I think we should have one background task who keeps polling the
tables from the queue and rebalance the table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
