somandal commented on code in PR #15496:
URL: https://github.com/apache/pinot/pull/15496#discussion_r2048069236
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/DefaultRebalancePreChecker.java:
##########
@@ -77,7 +78,9 @@ public Map<String, RebalancePreCheckerResult>
check(PreCheckContext preCheckCont
LOGGER.info("Start pre-checks for table: {} with rebalanceJobId: {}",
tableNameWithType, rebalanceJobId);
- Map<String, RebalancePreCheckerResult> preCheckResult = new HashMap<>();
+ // Right now pre-check items are done sequentially. If pre-check items are
to be done in parallel, we should not
+ // use linked hash map but to sort the result in the end
+ Map<String, RebalancePreCheckerResult> preCheckResult = new
LinkedHashMap<>();
Review Comment:
@Jackie-Jiang My main concern with changing the type here is due to the UI
side, this will break the UI and we'll need an additional fix. This is not a
very urgent change IMO, we can wait to tackle the `List` change and UI change
together once we get more important changes in, wdyt @J-HowHuang?
Also we have pretty exhaustive tests that will catch scenarios such as:
- If a different check overrides an existing one, tests should fail as the
message won't match. Every new pre-check should add to the tests and update the
assert that checks the Set size
- If we do happen to run the same pre-check more than once, other than the
wasted overhead of running it multiple times, it should be fine to overwrite
since they're idempotent and independent of each other.
Do you have concerns of memory or performance with using `LinkedHashSet` vs.
`LinkedList`? Again it is pretty small, so don't think that overhead will
matter much.
--
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]
