You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "heyallencao (via GitHub)" <gi...@apache.org> on 2026/09/16 10:20:59 UTC
[PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
heyallencao opened a new pull request, #4373:
URL: https://github.com/apache/fluss/pull/4373
### Purpose
Closes #4055.
Expose coordinator-level metrics for monitoring rebalance progress, duration, and outcomes without polling the Admin API.
### Brief change log
- Add seven gauges covering rebalance activity, pending/completed/failed/timed-out buckets, total elapsed time, and the current bucket's elapsed time.
- Add three counters for successfully completed, failed, and canceled rebalance rounds.
- Register the metrics only on the active coordinator and unregister them when leadership is lost. Retain the existing coordinator scope without additional table or bucket labels.
- Reuse the existing task maps, state, and timestamps, and add regression tests and metric documentation.
**Outcome semantics**
A round with any failed or timed-out buckets increments the failure counter. The existing Admin API's `COMPLETED` status means that execution has finished, not necessarily that every bucket succeeded; this PR does not change that API behavior.
An empty plan counts as a successful round. The cancellation counter increments only when a running rebalance is canceled; repeated cancellation does not increment it again.
**Leadership changes and recovery**
Counters accumulate within a coordinator leader term and reset on failover.
Bucket-outcome gauges include only results observed in the current leader term. They remain available after completion or cancellation and are cleared when the next round is registered. Since per-bucket outcomes are not persisted, recovering an already finished round leaves these gauges at zero and does not increment round counters. This avoids reporting historical failures as successes. These zeros mean that no outcomes were observed in the current term, not that the historical round had no failures.
### Tests
- `RebalanceManagerTest`: all 16 tests passed, covering progress, duration, success, failure, timeout, cancellation, historical task recovery, and metric lifecycle.
- `RebalanceManagerITCase`: both integration tests passed.
- `git diff --check` passed.
### API and Format
No changes to public client APIs, RPC messages, persisted rebalance task format, or rebalance execution semantics.
This adds monitoring metrics. Their leader-term lifetime and recovery behavior are documented.
### Documentation
Add a **Rebalance Metrics** section to the monitoring documentation, describing metric names, types, meanings, and behavior during leadership changes and task recovery.
### Generative AI disclosure
- [x] Generative AI tools were used: OpenAI Codex assisted with implementation, tests, code review, and this description.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6358581583
Thanks @heyallencao, this looks good to me. LGTM π
One small optional suggestion, since metric names become hard to change once dashboards depend on them: all the other new metrics start with "rebalance", but the in-flight bucket duration doesn't. Something like `rebalanceInflightBucketDurationMs` would make it clear on a dashboard that it belongs to rebalancing. Happy to merge as is if you'd rather keep it.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5792432691
Thanks for raising this. These counters are scoped to each coordinator process. A standby reports its own accumulated counts, which are zero if it has never processed a rebalance. When it becomes leader, it does not inherit the previous leader's counts. A process restart also resets its counters.
This is the intended per-process semantic, so these metrics do not provide a cluster-wide cumulative total that remains continuous across coordinators. The existing persisted rebalance state is not sufficient to reconstruct such a total accurately. If a cluster-wide cumulative total is needed, I think it should be defined as a separate metric contract, with its persistence semantics made explicit.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6359072532
Thanks @polyzos! Agreed β I've renamed the metric to `rebalanceInflightBucketDurationMs` and updated the constant, registration, tests, and documentation consistently.
Local validation passed: all 19 `RebalanceManagerTest` cases, Checkstyle, and the changed modules' Spotless/license checks.
The previous revision's core Java CI, connectors, documentation, and license checks passed. The Python 3.9 integration job failed before running tests because `actions/setup-python` could not find Python 3.9 x64 for Ubuntu 26.04 ([log](https://github.com/apache/fluss/actions/runs/39731324453/job/120091918046)); the Python 3.10β3.12 jobs were cancelled. This appears to be a runner/Python availability issue. I'll leave the workflow configuration outside this PR.
Thanks again for the review!
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub) via issues" <is...@fluss.apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6658446918
> @heyallencao LGTM π Please rebase so we can merge.
@polyzos Thanks for the review! Rebased onto the latest main β conflicts
resolved as unions of the upstream refactor and the metrics changes.
Verified locally: RebalanceManagerTest 19/19 passed, spotless + checkstyle
clean. Ready to merge once CI is green.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao closed pull request #4373: [coordinator] Add rebalance progress and outcome metrics
URL: https://github.com/apache/fluss/pull/4373
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5792539410
> Hello! Amazing contribution! Thank you!
>
> I have a question, with HA enabled, how does a standby keeps up with the old state of metrics? For example, if the count is 5, a new standby would start from 0 right? Is this a wanted semantic? So, one see that coordinator ID is different and no problem? Or we expect this metric to stay consistent through time and we should store that in ZK?
>
> If so, I think the state can be gathered already from ZK as per #3985
Thanks for raising this. These counters are scoped to each coordinator process. A standby reports its own accumulated counts, which are zero if it has never processed a rebalance. When it becomes leader, it does not inherit the previous leader's counts. A process restart also resets its counters.
This is the intended per-process semantic, so these metrics do not provide a cluster-wide cumulative total that remains continuous across coordinators. The existing persisted rebalance state is not sufficient to reconstruct such a total accurately. If a cluster-wide cumulative total is needed, I think it should be defined as a separate metric contract, with its persistence semantics made explicit.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "morazow (via GitHub)" <gi...@apache.org>.
morazow commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5715133985
Perfect!
Another suggestion:
Could we give the rebalance metrics the same lifetime as the coordinator process? We could introduce a `RebalanceMetrics` class owned by `CoordinatorMetricGroup` and register the counters and gauges once.
Then each `RebalanceManager` would bind/attach itself when created and unbind on close. The gauges would read from the current manager and return zero on standby, while counters would accumulate across leader terms within the same process.
This would avoid resetting counters and unregistering/re-registering metrics on leadership changes, and remove the need for a separate RebalanceMetricGroup:
- counters accumulate over every leader term the process serves and restart only when the process restarts
- gauges stay registered and report 0 on a standby, which is a meaningful value rather than an absent series
- getOrAddRebalanceMetricGroup, the RebalanceMetricGroup inner class and the CoordinatorMetricGroup.close() override all go away
Rough structure as below:
```java
public class RebalanceMetrics {
private final Counter rebalancesCompleted = new ThreadSafeSimpleCounter();
// ...
/** The manager of the current leader term, or null while this server is not the leader. */
private volatile @Nullable RebalanceManager current;
public RebalanceMetrics(MetricGroup metricGroup) {
metricGroup.counter(MetricNames.REBALANCES_COMPLETED_TOTAL, rebalancesCompleted);
// ...
metricGroup.gauge(
MetricNames.REBALANCE_BUCKETS_PENDING,
() -> readCurrent(RebalanceManager::pendingBucketCount));
// ...
}
private long readCurrent(ToLongFunction<RebalanceManager> read) {
RebalanceManager manager = current;
return manager == null ? 0L : read.applyAsLong(manager);
}
void bind(RebalanceManager manager) {
current = manager;
}
void unbind(RebalanceManager manager) {
// a late close from an ended term must not detach the manager that replaced it
if (current == manager) {
current = null;
}
}
}
```
RebalanceManager then takes `coordinatorMetricGroup.getRebalanceMetrics()`, calls `metrics.bind(this)` in the constructor and `metrics.unbind(this)` in close(), and increments through `metrics.incRebalancesCompleted()` and friends.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao closed pull request #4373: [coordinator] Add rebalance progress and outcome metrics
URL: https://github.com/apache/fluss/pull/4373
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub) via issues" <is...@fluss.apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6622081948
> Thanks @heyallencao! The changes look good to me.
>
> **One request before merging:** could you remove the new **Mockito** usage in RebalanceManagerTest? The project prefers real objects or small **Testing*** implementations. The test class already builds a real event processor you can reuse, and a minimal recording registry would cover the register/unregister checks.
>
> Optional nit: the new metrics table in the docs leaves out the Scope/Infix columns that the other tables have.
>
> Thanks!
Thanks for the review. Both comments are addressed in commit 47ef219da:
Removed Mockito from RebalanceManagerTest β metric registration is now verified with a minimal hand-written RecordingMetricRegistry, and the remaining mock(CoordinatorEventProcessor) calls are replaced with real processors built via the existing buildCoordinatorEventProcessor() helper.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5739059613
Thanks @morazow! I've addressed all six suggestions:\n\n- Documented the concurrency model of and marked it .\n- Added the null check for .\n- Ensured is closed through during coordinator shutdown.\n- Replaced repeated scans of finished results with per-status counters.\n- Reset those counters when registering a new rebalance.\n- Incremented the corresponding counter when a bucket finishes.\n\nI also did another self-review. All 19 tests pass, along with Checkstyle, Spotless, and RAT checks. Thanks again for the thoughtful suggestions!
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "morazow (via GitHub)" <gi...@apache.org>.
morazow commented on code in PR #4373:
URL: https://github.com/apache/fluss/pull/4373#discussion_r4046093087
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceMetrics.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.fluss.server.coordinator.rebalance;
+
+import org.apache.fluss.metrics.Counter;
+import org.apache.fluss.metrics.MetricNames;
+import org.apache.fluss.metrics.ThreadSafeSimpleCounter;
+import org.apache.fluss.metrics.groups.MetricGroup;
+
+import javax.annotation.Nullable;
+
+import java.util.function.ToLongFunction;
+
+import static org.apache.fluss.cluster.rebalance.RebalanceStatus.COMPLETED;
+import static org.apache.fluss.cluster.rebalance.RebalanceStatus.FAILED;
+import static org.apache.fluss.cluster.rebalance.RebalanceStatus.TIMEOUT;
+
+/** Rebalance metrics registered for the lifetime of a coordinator server. */
Review Comment:
```suggestion
/**
* Rebalance metrics registered for the lifetime of a coordinator server.
*
* <p>The coordinator event thread binds and unbinds the {@link RebalanceManager} of the current
* leader term and increments the counters, while the metric reporter thread reads the gauges
* through {@link #readCurrent}. Both sides are safe to call concurrently.
*/
@ThreadSafe
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorEventProcessor.java:
##########
@@ -277,7 +277,11 @@ public CoordinatorEventProcessor(
this.internalListenerName = conf.getString(ConfigOptions.INTERNAL_LISTENER_NAME);
this.rebalanceManager =
Review Comment:
On shutdown of this class, we should ensure that rebalance manager is closed even if exceptions thrown from earlier closes.
In shutdown method:
```java
try {
clearOfflineLeaderRetryTask();
// close the event manager
coordinatorEventManager.close();
} finally {
rebalanceManager.close();
}
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceManager.java:
##########
@@ -144,17 +150,54 @@ public RebalanceManager(
ZooKeeperClient zkClient,
EventManager eventManager,
Clock clock,
+ RebalanceMetrics metrics,
ScheduledExecutorService timeoutChecker) {
this.eventProcessor = eventProcessor;
this.zkClient = zkClient;
this.eventManager = eventManager;
this.clock = clock == null ? SystemClock.getInstance() : clock;
this.timeoutChecker = timeoutChecker;
this.goalOptimizer = new GoalOptimizer();
+ this.metrics = metrics;
Review Comment:
```suggestion
this.metrics = checkNotNull(metrics, "metrics");
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceManager.java:
##########
@@ -144,17 +150,54 @@ public RebalanceManager(
ZooKeeperClient zkClient,
EventManager eventManager,
Clock clock,
+ RebalanceMetrics metrics,
ScheduledExecutorService timeoutChecker) {
this.eventProcessor = eventProcessor;
this.zkClient = zkClient;
this.eventManager = eventManager;
this.clock = clock == null ? SystemClock.getInstance() : clock;
this.timeoutChecker = timeoutChecker;
this.goalOptimizer = new GoalOptimizer();
+ this.metrics = metrics;
+ }
+
+ long rebalanceInProgress() {
+ return rebalanceStatus == REBALANCING ? 1L : 0L;
+ }
+
+ long pendingBucketCount() {
+ return inProgressRebalanceTasks.size();
+ }
+
+ long rebalanceDurationMs() {
+ return rebalanceStatus == REBALANCING
+ ? Math.max(0L, clock.milliseconds() - registerTime)
+ : 0L;
+ }
+
+ long finishedBucketCount(RebalanceStatus status) {
+ if (!bucketResultsAvailable) {
+ return 0L;
+ }
+ return finishedRebalanceTasks.values().stream()
Review Comment:
I wonder if we can still improve this, instead of iterating for each gauge. Also the `finishedRebalanceTasks` are never cleared.
Suggestion: Add a map to count the statuses as the buckets finish.
```
/**
* How many buckets of the current rebalance ended in each status.
*/
private final Map<RebalanceStatus, AtomicLong> finishedBucketCounts = newFinishedBucketCounts();
private static Map<RebalanceStatus, AtomicLong> newFinishedBucketCounts() {
Map<RebalanceStatus, AtomicLong> counts = new EnumMap<>(RebalanceStatus.class);
for (RebalanceStatus status : RebalanceStatus.values()) {
counts.put(status, new AtomicLong());
}
return counts;
}
```
And on iteration counts replace:
```java
return finishedBucketCounts.get(status).get();
```
What do you think?
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceManager.java:
##########
Review Comment:
Because of above suggestion:
```suggestion
finishedBucketCounts.get(statusForBucket).incrementAndGet();
// Clear gate (bucket) first, then data (startMs).
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceManager.java:
##########
@@ -194,11 +237,12 @@ public void registerRebalance(
Map<TableBucket, RebalancePlanForBucket> rebalancePlan,
RebalanceStatus newStatus) {
checkNotClosed();
- registerTime = System.currentTimeMillis();
+ registerTime = clock.milliseconds();
// first clear all exists tasks.
inProgressRebalanceTasks.clear();
inProgressRebalanceTasksQueue.clear();
finishedRebalanceTasks.clear();
Review Comment:
For above suggestion:
```suggestion
finishedRebalanceTasks.clear();
finishedBucketCounts.values().forEach(count -> count.set(0L));
```
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "morazow (via GitHub)" <gi...@apache.org>.
morazow commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5757090067
Thanks @heyallencao! Looks good π
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub) via issues" <is...@fluss.apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6605782642
> @heyallencao thanks for the great contribution, I am going to raise attention to committers to land this π€
ThanksοΌ
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5714061589
Thanks for the review, @morazow! All three comments are addressed. The 18 targeted rebalance tests and style checks pass.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6333929675
Hi @morazow @affo, thanks again for reviewing and approving this PR. I've merged current main into the branch in 75ca530b0e0d43ddf7b4095ae3dc7ec7324476fc, resolved the coordinator test-helper conflict, and updated the constructor-failure regression test to retain the remote storage configuration now required by main.
Local validation with JDK 17 and Maven 3.9.14:
- Passed `mvn -B -pl fluss-server -am verify '-Dtest=*Rebalance*Test,*Coordinator*Test,*Metric*Test' -Dsurefire.failIfNoSpecifiedTests=false`, including all 19 `RebalanceManagerTest` cases; zero Checkstyle violations.
- Passed `mvn -B -pl fluss-common,fluss-server spotless:check validate`.
- Full `mvn -B verify` stopped in `LakeStorageTest.testWithPluginManager` with Mockito's "Cannot call abstract real method on java object!" error. That test and its lake-storage implementation are unchanged by this PR, so I have not modified them here.
Could you please take another look and help move this toward merge? The new GitHub Actions runs also conclude `action_required` (for example, [CI](https://github.com/apache/fluss/actions/runs/39731324021)); could a maintainer please check and approve the workflows if needed? Thanks!
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "polyzos (via GitHub) via issues" <is...@fluss.apache.org>.
polyzos commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6606007743
Thanks @heyallencao! The changes look good to me.
One request before merging: could you remove the new **Mockito** usage in RebalanceManagerTest? The project prefers real objects or small **Testing*** implementations. The test class already builds a real event processor you can reuse, and a minimal recording registry would cover the register/unregister checks.
Optional nit: the new metrics table in the docs leaves out the Scope/Infix columns that the other tables have.
Thanks!
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub) via issues" <is...@fluss.apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6568383470
@morazow @affo The latest update merges the current `main` into this PR and resolves the merge conflict. GitHub now reports the branch as up to date and mergeable. Could you please take another look at the latest commit `83c54bca` and confirm whether your approvals still stand? The six Actions runs for this fork PR are currently `action_required` with no jobs started. Could a maintainer approve the workflows so CI can run? Thanks!
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5714276199
> Thanks @heyallencao π
>
> Added minor nits
Thanks for the review, @morazow! All three comments are addressed. The 18 targeted rebalance tests and style checks pass.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "morazow (via GitHub)" <gi...@apache.org>.
morazow commented on code in PR #4373:
URL: https://github.com/apache/fluss/pull/4373#discussion_r4036141284
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceManager.java:
##########
@@ -194,11 +253,12 @@ public void registerRebalance(
Map<TableBucket, RebalancePlanForBucket> rebalancePlan,
RebalanceStatus newStatus) {
checkNotClosed();
- registerTime = System.currentTimeMillis();
+ registerTime = clock.milliseconds();
Review Comment:
I'd suggest to refactor other usages of system ms to clock milliseconds π€
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/rebalance/RebalanceManager.java:
##########
@@ -407,14 +470,21 @@ private void completeRebalance() {
LOG.error("Error when update rebalance plan from zookeeper.", e);
}
+ if (bucketResultsAvailable) {
+ if (finishedBucketCount(FAILED) > 0 || finishedBucketCount(TIMEOUT) > 0) {
Review Comment:
For these, maybe we can add another method using `anyMatch` short circuit, e.g,
```
private boolean hasFinishedBucket(RebalanceStatus status) {
return finishedRebalanceTasks.values().stream().anyMatch(r -> r.status() == status);
}
```
this deliberately doesn't use the `bucketResultsAvailable` since caller already checks it.
##########
website/docs/maintenance/observability/monitor-metrics.md:
##########
@@ -449,6 +449,32 @@ Some metrics might not be exposed when using other JVM implementations (e.g. IBM
</tbody>
</table>
+#### Rebalance Metrics
+
+Rebalance metrics use the `coordinator` scope with no additional infix or per-table/per-bucket labels.
+They are registered only on the active coordinator and removed when it loses leadership.
+Gauges describe the most recently registered rebalance. Finished bucket counts include only outcomes
+observed in the current coordinator leader term, remain available after completion or cancellation,
+and are cleared when the next rebalance is registered. Recovering a previously finished rebalance
+leaves these counts at 0 because per-bucket outcomes are not persisted. These zeros indicate no
+outcomes observed by this leader, not that the historical rebalance had no failures or timeouts.
+Counters accumulate across rebalances within a coordinator leader term and reset on failover.
+Recovering a previously finished rebalance does not increment the counters; a recovered unfinished
+rebalance is counted when it finishes in the new leader term.
+
+| Metrics | Description | Type |
Review Comment:
The rest of the document somehow used the HTML table format, let's use it to keep consistent π
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5792466705
> Thanks for raising this. These counters are scoped to each coordinator process. A standby reports its own accumulated counts, which are zero if it has never processed a rebalance. When it becomes leader, it does not inherit the previous leader's counts. A process restart also resets its counters.
>
> This is the intended per-process semantic, so these metrics do not provide a cluster-wide cumulative total that remains continuous across coordinators. The existing persisted rebalance state is not sufficient to reconstruct such a total accurately. If a cluster-wide cumulative total is needed, I think it should be defined as a separate metric contract, with its persistence semantics made explicit.
> Hello! Amazing contribution! Thank you!
>
> I have a question, with HA enabled, how does a standby keeps up with the old state of metrics? For example, if the count is 5, a new standby would start from 0 right? Is this a wanted semantic? So, one see that coordinator ID is different and no problem? Or we expect this metric to stay consistent through time and we should store that in ZK?
>
> If so, I think the state can be gathered already from ZK as per #3985
Thanks for raising this. These counters are scoped to each coordinator process. A standby reports its own accumulated counts, which are zero if it has never processed a rebalance. When it becomes leader, it does not inherit the previous leader's counts. A process restart also resets its counters.
This is the intended per-process semantic, so these metrics do not provide a cluster-wide cumulative total that remains continuous across coordinators. The existing persisted rebalance state is not sufficient to reconstruct such a total accurately. If a cluster-wide cumulative total is needed, I think it should be defined as a separate metric contract, with its persistence semantics made explicit.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "polyzos (via GitHub) via issues" <is...@fluss.apache.org>.
polyzos commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6624970351
@heyallencao LGTM π Please rebase so we can merge.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6122177324
> @heyallencao what you say makes sense, metrics are indeed scoped on a per-pod basis, so there is no chance to mis-interpret them π€
thanks
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub) via issues" <is...@fluss.apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6628493617
> @heyallencao LGTM π Please rebase so we can merge.
@polyzos Thanks for the review! Rebased onto the latest `main` β conflicts
resolved as unions of the upstream refactor and the metrics changes.
Verified locally: `RebalanceManagerTest` 19/19 passed, spotless + checkstyle
clean. Ready to merge once CI is green.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "affo (via GitHub) via issues" <is...@fluss.apache.org>.
affo commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-6580470036
@heyallencao thanks for the great contribution, I am going to raise attention to committers to land 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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5792532939
Thanks for raising this. These counters are scoped to each coordinator process. A standby reports its own accumulated counts, which are zero if it has never processed a rebalance. When it becomes leader, it does not inherit the previous leader's counts. A process restart also resets its counters.
This is the intended per-process semantic, so these metrics do not provide a cluster-wide cumulative total that remains continuous across coordinators. The existing persisted rebalance state is not sufficient to reconstruct such a total accurately. If a cluster-wide cumulative total is needed, I think it should be defined as a separate metric contract, with its persistence semantics made explicit.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [coordinator] Add rebalance progress and outcome metrics [fluss]
Posted by "heyallencao (via GitHub)" <gi...@apache.org>.
heyallencao commented on PR #4373:
URL: https://github.com/apache/fluss/pull/4373#issuecomment-5714268689
> Thanks for the review, @morazow! All three comments are addressed. The 18 targeted rebalance tests and style checks pass.
Thanks for the review, @morazow! All three comments are addressed. The 18 targeted rebalance tests and style checks pass.
--
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: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org