This is an automated email from the ASF dual-hosted git repository.
SteNicholas pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/celeborn.git
The following commit(s) were added to refs/heads/main by this push:
new 784f470c5d [CELEBORN-2497][TEST] Fix gauge snapshot race in Master
metrics tests
784f470c5d is described below
commit 784f470c5d016a3ad784f5807fcf9069aa6b36d3
Author: zhengtao <[email protected]>
AuthorDate: Thu Oct 22 20:16:28 2026 +0800
[CELEBORN-2497][TEST] Fix gauge snapshot race in Master metrics tests
### What changes were proposed in this pull request?
Capture a single `source.gauges()` snapshot in the `exportedGauge` helpers
used by `RatisMasterStatusSystemSuiteJ` and `DefaultMetaSystemSuiteJ`, then use
that snapshot for both the emptiness check and the gauge read.
JIRA: https://issues.apache.org/jira/browse/CELEBORN-2497
### Why are the changes needed?
`source.gauges()` creates a new list on each call. When `AppLost` removes
the last application between the existing `isEmpty()` and `head()` calls, the
latter can see an empty list and throw `NoSuchElementException: head of empty
list`.
This caused the [service (11) CI
failure](https://github.com/apache/celeborn/actions/runs/39541445031/job/119030128356)
in
`SSLRatisMasterStatusSystemSuiteJ.testClientMetricsReplicatedAcrossLeadershipChanges`.
Using one snapshot keeps each poll safe while later polls can still observe
removal. The ordinary Master helper uses the same pattern and is updated
consistently.
### Does this PR resolve a correctness bug?
- [ ] Yes
This fixes test flakiness only.
### Does this PR introduce _any_ user-facing change?
- [ ] Yes
### How was this patch tested?
- A local deterministic probe removes the last application immediately
after obtaining the first snapshot. Both original helpers throw
`NoSuchElementException`; both fixed helpers survive the removal and return
`null` on the next poll. The probe also checks the initially empty and
live-gauge cases.
- 24 tests passed on JDK 17 / Scala 2.12.18: all 22 tests in
`DefaultMetaSystemSuiteJ` plus
`testClientMetricsReplicatedAcrossLeadershipChanges` in the normal and SSL HA
suites:
```sh
build/mvn -pl master -am test \
'-Dtest=RatisMasterStatusSystemSuiteJ#testClientMetricsReplicatedAcrossLeadershipChanges,SSLRatisMasterStatusSystemSuiteJ#testClientMetricsReplicatedAcrossLeadershipChanges,DefaultMetaSystemSuiteJ'
\
-DwildcardSuites=none -Dsurefire.failIfNoSpecifiedTests=false
```
- `dev/reformat`: all 16 invocations succeeded.
- `git diff --check` passed.
Closes #3889 from zaynt4606/codex/fix-master-ha-gauge-snapshot.
Authored-by: zhengtao <[email protected]>
Signed-off-by: Nicholas Jiang <[email protected]>
---
.../service/deploy/master/clustermeta/DefaultMetaSystemSuiteJ.java | 7 +++++--
.../master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java | 7 +++++--
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git
a/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/DefaultMetaSystemSuiteJ.java
b/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/DefaultMetaSystemSuiteJ.java
index ed6eedcf91..983f2d6286 100644
---
a/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/DefaultMetaSystemSuiteJ.java
+++
b/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/DefaultMetaSystemSuiteJ.java
@@ -48,6 +48,7 @@ import org.apache.celeborn.common.meta.ApplicationMeta;
import org.apache.celeborn.common.meta.DiskInfo;
import org.apache.celeborn.common.meta.WorkerInfo;
import org.apache.celeborn.common.meta.WorkerStatus;
+import org.apache.celeborn.common.metrics.source.NamedGauge;
import org.apache.celeborn.common.network.CelebornRackResolver;
import org.apache.celeborn.common.quota.ResourceConsumption;
import org.apache.celeborn.common.rpc.RpcEndpointAddress;
@@ -1275,10 +1276,12 @@ public class DefaultMetaSystemSuiteJ {
/** Sum of the client gauge across apps as exported by the source, or null
if not exported. */
private static Long exportedGauge(ApplicationMetricsSource source) {
- if (source.gauges().isEmpty()) {
+ // Use one snapshot because application removal can empty the source
between reads.
+ scala.collection.immutable.List<NamedGauge<?>> gauges = source.gauges();
+ if (gauges.isEmpty()) {
return null;
}
- return ((Number) source.gauges().head().gauge().getValue()).longValue();
+ return ((Number) gauges.head().gauge().getValue()).longValue();
}
/** A metrics source that fails on every call, to exercise the apply-path
guardrails. */
diff --git
a/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java
b/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java
index 0656844c2a..ef63af7fe5 100644
---
a/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java
+++
b/master/src/test/java/org/apache/celeborn/service/deploy/master/clustermeta/ha/RatisMasterStatusSystemSuiteJ.java
@@ -46,6 +46,7 @@ import org.apache.celeborn.common.identity.UserIdentifier;
import org.apache.celeborn.common.meta.DiskInfo;
import org.apache.celeborn.common.meta.WorkerInfo;
import org.apache.celeborn.common.meta.WorkerStatus;
+import org.apache.celeborn.common.metrics.source.NamedGauge;
import org.apache.celeborn.common.network.CelebornRackResolver;
import org.apache.celeborn.common.network.sasl.SaslCredentials;
import org.apache.celeborn.common.network.sasl.SaslTestBase;
@@ -899,10 +900,12 @@ public class RatisMasterStatusSystemSuiteJ {
/** Sum of the client gauge across apps as exported by the source, or null
if not exported. */
private static Long exportedGauge(ApplicationMetricsSource source) {
- if (source.gauges().isEmpty()) {
+ // Use one snapshot because application removal can empty the source
between reads.
+ scala.collection.immutable.List<NamedGauge<?>> gauges = source.gauges();
+ if (gauges.isEmpty()) {
return null;
}
- return ((Number) source.gauges().head().gauge().getValue()).longValue();
+ return ((Number) gauges.head().gauge().getValue()).longValue();
}
private static void waitForAllMasters(ApplicationMetricsSource[] sources,
Long expected)
