wuchong commented on code in PR #3400:
URL: https://github.com/apache/fluss/pull/3400#discussion_r3387486885
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorRequestBatch.java:
##########
@@ -428,6 +437,13 @@ private void sendNotifyLeaderAndIsrRequest(int
coordinatorEpoch) {
// coordinator will remove the sender for the
tablet server and mark all
// replica in the tablet server as offline. so, in
here, if encounter
// any error, we just ignore it.
+
+ // Clear pending state so the health API does not
report stale
+ // RED. The coordinator will detect actual server
death via
+ // heartbeat timeout and trigger re-election
separately.
+ for (TableBucket tb : buckets) {
+
coordinatorContext.clearPendingLeaderActivation(tb);
Review Comment:
Besides, `coordinatorContext.clearPendingLeaderActivation(...)` is called
inside the NotifyLeaderAndIsr send-failure callback, which runs on the RPC
completion thread (whenComplete is attached with no executor), not the single
coordinator event thread. CoordinatorContext is @NotThreadSafe and
pendingLeaderActivationBuckets is a plain HashSet that is concurrently
read/mutated on the event thread (e.g. addPendingLeaderActivation,
isLeaderActive, computeClusterHealth via AccessContextEvent). This is an
unsynchronized data race. The success branch right below correctly hands off
via eventManager.put(...) instead of touching the context directly — the
failure branch should do the same.
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorRequestBatch.java:
##########
@@ -428,6 +437,13 @@ private void sendNotifyLeaderAndIsrRequest(int
coordinatorEpoch) {
// coordinator will remove the sender for the
tablet server and mark all
// replica in the tablet server as offline. so, in
here, if encounter
// any error, we just ignore it.
+
+ // Clear pending state so the health API does not
report stale
+ // RED. The coordinator will detect actual server
death via
+ // heartbeat timeout and trigger re-election
separately.
+ for (TableBucket tb : buckets) {
+
coordinatorContext.clearPendingLeaderActivation(tb);
Review Comment:
When a NotifyLeaderAndIsr request to a follower fails, buckets still
contains the bucket whose pending activation was added for the actual leader.
Clearing every bucket here can remove that leader's pending state before the
leader has acknowledged the request, allowing getClusterHealth() to report
GREEN during a rolling upgrade while the new leader is not confirmed active.
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorContext.java:
##########
@@ -106,6 +106,13 @@ public class CoordinatorContext {
*/
private final Map<Integer, Set<TableBucket>> replicasOnOffline = new
HashMap<>();
+ /**
+ * Tracks buckets where a leader change has been dispatched (via
NotifyLeaderAndIsr) but not yet
+ * confirmed by the target server. A bucket enters this set when we send
the notification and
+ * leaves it when the target server successfully responds confirming it is
the leader.
+ */
+ private final Set<TableBucket> pendingLeaderActivationBuckets = new
HashSet<>();
Review Comment:
Suggest adding a gauge metric for `pendingLeaderActivationBuckets`.
This set is the core signal behind the RED status of the health API:
`isLeaderActive` returns false for any bucket in it, which makes the health API
report RED and blocks the rolling upgrade at the readiness gate. If a leader
bucket's activation never gets cleared (e.g. due to an exception or a missed
clear), the upgrade hangs at NotReady indefinitely, while operators only see a
pod stuck NotReady with no way to tell which bucket is stuck or for how long —
they'd have to dig through coordinator logs, which is expensive to diagnose.
Suggest registering a gauge in `CoordinatorMetricGroup` (e.g.
`pendingLeaderActivationCount`) backed by the existing read-only accessor
`coordinatorContext.getPendingLeaderActivationBuckets().size()`. This mirrors
the existing `gauge(MetricNames.BUCKET_COUNT, buckets::size)` in the same file
and is consistent in spirit with `offlineBucketCount` /
`replicasToDeleteCount`. A value that stays non-zero over time then becomes
directly alertable/observable on dashboards, turning "upgrade hangs" from a
hard-to-debug mystery into a visible curve.
##########
fluss-dist/pom.xml:
##########
@@ -263,8 +263,24 @@
<finalName>${project.artifactId}-${project.version}</finalName>
<artifactSet>
<includes>
- <!-- only include log4j-api to package
source and javadoc to make sonatype happy -->
-
<include>org.apache.logging.log4j:log4j-api</include>
+ <!--
+ IMPORTANT: do NOT include log4j-api
here.
Review Comment:
we should revert this, otherwise, release will fail on sonatype.
--
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]
