somandal commented on code in PR #15266:
URL: https://github.com/apache/pinot/pull/15266#discussion_r2022144726
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/ZkBasedTableRebalanceObserver.java:
##########
@@ -68,31 +70,67 @@ public ZkBasedTableRebalanceObserver(String
tableNameWithType, String rebalanceJ
@Override
public void onTrigger(Trigger trigger, Map<String, Map<String, String>>
currentState,
- Map<String, Map<String, String>> targetState) {
+ Map<String, Map<String, String>> targetState, RebalanceContext
rebalanceContext) {
boolean updatedStatsInZk = false;
_controllerMetrics.setValueOfTableGauge(_tableNameWithType,
ControllerGauge.TABLE_REBALANCE_IN_PROGRESS, 1);
+ TableRebalanceProgressStats.RebalanceStateStats latest;
+ TableRebalanceProgressStats.RebalanceProgressStats latestProgress;
switch (trigger) {
case START_TRIGGER:
- updateOnStart(currentState, targetState);
+ updateOnStart(currentState, targetState, rebalanceContext);
trackStatsInZk();
updatedStatsInZk = true;
break;
// Write to Zk if there's change since previous stats computation
case IDEAL_STATE_CHANGE_TRIGGER:
- TableRebalanceProgressStats.RebalanceStateStats latest =
- getDifferenceBetweenTableRebalanceStates(targetState,
currentState);
+ latest = getDifferenceBetweenTableRebalanceStates(targetState,
currentState);
+ latestProgress = calculateOverallProgressStats(targetState,
+ currentState, rebalanceContext,
Trigger.IDEAL_STATE_CHANGE_TRIGGER, _tableRebalanceProgressStats);
if
(TableRebalanceProgressStats.statsDiffer(_tableRebalanceProgressStats.getCurrentToTargetConvergence(),
- latest)) {
- _tableRebalanceProgressStats.setCurrentToTargetConvergence(latest);
+ latest) || TableRebalanceProgressStats.progressStatsDiffer(
+ _tableRebalanceProgressStats.getRebalanceProgressStatsOverall(),
latestProgress)) {
+ if (TableRebalanceProgressStats.statsDiffer(
+
_tableRebalanceProgressStats.getExternalViewToIdealStateConvergence(), latest))
{
+ _tableRebalanceProgressStats.setCurrentToTargetConvergence(latest);
+ }
+ if (TableRebalanceProgressStats.progressStatsDiffer(
+ _tableRebalanceProgressStats.getRebalanceProgressStatsOverall(),
latestProgress)) {
+
_tableRebalanceProgressStats.setRebalanceProgressStatsOverall(latestProgress);
+ }
trackStatsInZk();
updatedStatsInZk = true;
}
break;
case EXTERNAL_VIEW_TO_IDEAL_STATE_CONVERGENCE_TRIGGER:
latest = getDifferenceBetweenTableRebalanceStates(targetState,
currentState);
+ latestProgress = calculateOverallProgressStats(targetState,
+ currentState, rebalanceContext,
Trigger.EXTERNAL_VIEW_TO_IDEAL_STATE_CONVERGENCE_TRIGGER,
+ _tableRebalanceProgressStats);
if (TableRebalanceProgressStats.statsDiffer(
-
_tableRebalanceProgressStats.getExternalViewToIdealStateConvergence(), latest))
{
-
_tableRebalanceProgressStats.setExternalViewToIdealStateConvergence(latest);
+
_tableRebalanceProgressStats.getExternalViewToIdealStateConvergence(), latest)
+ || TableRebalanceProgressStats.progressStatsDiffer(
+
_tableRebalanceProgressStats.getRebalanceProgressStatsCurrentStep(),
latestProgress)) {
+ if (TableRebalanceProgressStats.statsDiffer(
+
_tableRebalanceProgressStats.getExternalViewToIdealStateConvergence(), latest))
{
+
_tableRebalanceProgressStats.setExternalViewToIdealStateConvergence(latest);
+ }
+ TableRebalanceProgressStats.RebalanceProgressStats lastStepStats =
+
_tableRebalanceProgressStats.getRebalanceProgressStatsCurrentStep();
+ if (TableRebalanceProgressStats.progressStatsDiffer(lastStepStats,
latestProgress)) {
+ _tableRebalanceProgressStats.setRebalanceProgressStatsOverall(
+
updateOverallProgressStatsFromStep(_tableRebalanceProgressStats, lastStepStats,
latestProgress));
+
_tableRebalanceProgressStats.setRebalanceProgressStatsCurrentStep(latestProgress);
+ }
+ trackStatsInZk();
+ updatedStatsInZk = true;
+ }
+ break;
+ case NEXT_ASSINGMENT_CALCULATION_TRIGGER:
+ latestProgress = calculateOverallProgressStats(targetState,
Review Comment:
if `bestEfforts=false`, if there is any error in EV-IS convergence (segments
in ERROR state, or timeout in convergence), the rebalance will be aborted.
Similarly, if say we cancel rebalance, or other exceptions occur rebalance will
be aborted. The stats will not be updated and left in the intermediate state in
that case, since the rebalance job is no longer running. if `RebalanceChecker`
retries the rebalance for the table, it'll start a new job and the new job will
have new stats for tracking.
While thinking about this, I just realized that the stats code I have
currently doesn't handle stats well when `bestEfforts=true`, since in this
scenario, we move to the next rebalance step without waiting for EV-IS
convergence to complete, or when we have `ERROR` state segments. In this case,
the stats may not reflect correctly because the comparisons for various
triggers are done between different states:
- EV-IS convergence checks EV and IS
- Other triggers compare the current assignment and target assignment, both
of which are related to IS only
Due to the above, if any segments are carried over from the last step, we
can start seeing -ve stats or weird stats values.
Does the above answer your question?
I'm working on a possible fix, to track these additional adds / deletes
carried over from the previous step separately. Will let you know when I've
updated the code to handle this.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
