fix(#6267): a peer address that identifies nobody says so, and a membership test stops poisoning its own teardown - #6284
Conversation
…ership test stops poisoning its own teardown Five follow-ups from #6221 / #6226. 1. A withheld peer-to-peer endpoint is now reported. getUnambiguousPeerHttpAddress and its HTTPS twin refuse an address two peers resolve to by returning null (#6202), and every caller decided for itself whether to say anything. Neither existing warning covers it: the derive warnings fire whenever an address is derived at all, which a healthy homogeneous StatefulSet also does. There is now a one-time WARNING per protocol - modelled on warnAmbiguousRouting (#6183), but for the peer-to-peer endpoints - naming the peers that could not be told apart, the address they share and the field to declare. HTTP and HTTPS have separate latches. 2. GET /api/v1/cluster reports each peer's httpAddress and, only when it is not that peer's alone, httpAddressAmbiguous: true. Studio renders it as a warning line on the node's card, and the OpenAPI spec documents both. A correctly declared cluster carries neither. 3. The presence matrix (?presence=true) dialled the best-effort address and attributed the answer to the peer - the same unattended dial #6221 fixed for the verify endpoint, with the same failure: every peer was queried on the leader's own endpoint and reported the leader's databases as its own, so the matrix showed every database present everywhere. It resolves through PeerDialAddress now and reports a peer it cannot identify as unreachable, with the reason logged. RaftClusterStatusExporter.exportClusterStatus(), a second cluster-status JSON builder that nothing called, is removed rather than taught about this: the live endpoint is GetClusterHandler, and an unreachable second view of one cluster is how two views drift apart. 4. RESYNC_RETRY_TIMEOUT_MS drops from 120s to 30s, set from the instrument #6226 added rather than from a suspicion: across nine full ha-integration-tests runs (235 tests each) not one wait exceeded the 10s report threshold, and the slowest of the ten classes that use those helpers took 53s wall-clock for the whole class, cluster startup and teardown included. 30s is what the rest of the class already treats as long enough - waitForReplicationIsCompleted, waitAllReplicasAreConnected and the leader-election wait all use it. The report threshold drops to 5s to keep the same resolution for the next cut. 5. DynamicMembershipTest no longer leaves its own teardown holding a peer it evicted to a replica's contract. Each method already gets its own cluster (@BeforeEach/@AfterEach, Raft storage under the deleted database directory), so the issue's shared-cluster premise does not apply and two of the three reported failures were the leaderless-window assert that #6226 already fixed by making findLeaderIndex wait. The third is real and now has a regression test: the base class waits for, and compares, exactly the servers getServerToCheck() names, turning a 30s-per-evicted-server timeout and a DatabaseAreNotIdentical charged to endTest into neither (152s -> 77s for the class, with one more test). Seven await().until(() -> findLeaderIndex() >= 0) wrappers that #6226 made redundant are gone, and three copies of "only the servers still running" collapse into BaseRaftHATest.startedServers(). Tests: new Issue6267AmbiguousPeerAddressWarningTest (3, warning fires once per protocol, names the peers, stays silent on a correct cluster, HTTPS not muted by HTTP) and Issue6267AmbiguousAddressVisibilityIT (3, the status flag on an ambiguous and on a correct cluster, and the presence matrix reporting a peer it cannot identify as unreachable - confirmed failing against the pre-fix dial). New DynamicMembershipTest#removedPeerIsNotHeldToTheClusterConsistencyCheck confirmed failing without the getServerToCheck fix with exactly the reported "DatabaseAreNotIdentical Types: DB1 5 <> DB2 6" at endTest. Run locally: full ha-raft unit lane (918), the 18 IT classes reached by these changes (35 tests) including all ten that use the shortened budget, server PluginApiSpecTest, and a full reactor build.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -15 |
🟢 Coverage 96.77% diff coverage · -6.50% coverage variation
Metric Results Coverage variation ✅ -6.50% coverage variation Diff coverage ✅ 96.77% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (6b20fb1) 159115 125535 78.90% Head commit (1e186b1) 191455 (+32340) 138602 (+13067) 72.39% (-6.50%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#6284) 62 60 96.77% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
ReviewI read through the full diff ( Overall this is a careful, well-documented follow-up. The reasoning in the PR description matches what's actually in the diff (e.g. confirmed Code quality
Potential issues (minor, non-blocking)
Test coverageStrong. SecurityNo concerns. The new PerformanceChanges are confined to status/reporting paths (cluster status endpoint, opt-in presence fan-out, one-time warnings) and test infrastructure - nothing on the replication/write hot path, consistent with the CLAUDE.md performance mandate. Nice cleanup on the dead |
…roup, not once per peer Code review follow-up on the two actionable points. 1. GetClusterHandler asked getPeerHttpAddress and getUnambiguousPeerHttpAddress per peer, and answering the second means resolving every peer to see who else claims the address - so a /api/v1/cluster request resolved the group twice per peer, O(peers²) for a question that is O(peers). Cluster sizes make that harmless today, which is why it is worth removing before the shape is copied somewhere it is not. RaftHAServer.getPeerHttpEndpoints() answers it for the whole group in one pass, returning each peer's address with its verdict; the handler does a lookup. It shares claimsByAddress/identifiesOnePeer with the per-peer accessors, so it cannot answer differently, and a new test asserts exactly that for every peer of an ambiguous and of a correctly declared cluster. An ambiguity seen here trips the same one-time warning. 2. warnAmbiguousPeerAddress's javadoc now says what the latch does NOT do: it is per JVM and never rearms, so a membership change that makes a different pair of peers collide is not logged again. The line names the peers that tripped it first; httpAddressAmbiguous on GET /api/v1/cluster is recomputed per request and is the current answer. The third point (the latch convention itself) was raised as consistent-not-a-problem and is unchanged, as is the timeout measurement. Run: ha-raft unit lane (919), Issue6267AmbiguousAddressVisibilityIT.
ReviewThorough PR with an unusually well-documented rationale (including CI-run evidence for the timeout reduction). Went through the Code quality
Potential bugs
Performance
Security
Test coverage
Overall this looks solid and ready modulo the minor items above. |
…a stable order Round 2 of the code review. Two points taken, one declined. 1. The one-time warning reported a single colliding address - whichever the pass saw last - so a cluster with two independent colliding pairs got one of them named and the other silently absorbed. Since the latch never rearms, that pair would then be withheld with the log permanently quiet, and an operator who declared the two ports the line named would still have a broken cluster. It now names every collision the pass found, grouped by address, and takes the latch only once there is something to say. The groups and the peers within them are sorted, so the same misconfiguration produces the same line whichever node logs it - Ratis hands the group over in its own order, which is what made the first version of the new test order-dependent. 2. Dropped the JSONArray/JSONObject/ArcadeDBServer imports that exportClusterStatus() left behind in RaftClusterStatusExporter. Declined: routing buildPresenceMatrix's dial through the precomputed getPeerHttpEndpoints map instead of PeerDialAddress.resolve(). It would save one O(peers) resolution per peer on a path that then makes a network round trip per peer with a 5 s timeout - and it would do it by hand-assembling the guard (self-dial check, the HTTPS endpoint, the refusal text) that #6202/#6221 consolidated into one helper precisely because call sites had drifted apart. That trade is the wrong way round; the endpoint map is for reporting, PeerDialAddress is for dialling. Tests: oneWarningNamesEveryCollisionItFound covers two independent colliding pairs in one line. Run: ha-raft unit lane (920), Issue6267AmbiguousAddressVisibilityIT, Issue6221VerifyFanOutGuardIT.
ReviewI read through the full diff (docs, Correctness - looks solid
Minor nit
Design points already discussed in the PR description
Test coverageGood coverage for the behavior change: Nothing blocking found. Nice use of |
… SuperNodeConcurrentAppendHABenchmark Round 3 of the code review, its only point: the class's sole use of TimeUnit was the Awaitility.await(...).atMost(30, TimeUnit.SECONDS) wrapper this PR removed.
ReviewI read through the full diff (main-code changes in Overall this is a well-scoped, well-documented change. A few observations, nothing blocking: Correctness
Minor / non-blocking nits
Test coverageGood, new behavior is covered from three angles: a unit test against a detached Security / performanceNo security concerns; the new code doesn't touch authentication/authorization paths and only exposes address/ambiguity metadata already implicitly derivable from log warnings. I did not find any functional bugs in this diff. |
…iction test is not racy Round 4 of the code review; it found no functional issue, both notes are wording. - The javadoc called the warning latch "per JVM". It is a field on this RaftHAServer, like every other warn-once latch in the class - true per process where there is one server, and not true in a test that builds several, which is exactly what the new unit tests rely on. Says that now. - The review flagged that removedPeerIsNotHeldToTheClusterConsistencyCheck would be flaky if removePeer() did not block until the reconfiguration commits. It does: RaftClusterManager.removePeer drives Ratis's blocking admin().setConfiguration(...) and returns only on a successful reply. Written down on evict(), where the next reader will ask the same question - the assertions that follow it read committed membership, and a write issued after it cannot still reach the evicted peer. Run: DynamicMembershipTest (7), Issue6267AmbiguousPeerAddressWarningTest (5).
ReviewI went through the diff (docs, Correctness (verified, no bugs found)
Minor / non-blocking
Test coverage I did not run the build in this environment (sandboxed, no Nice work on the investigation write-up in the PR description too - the correction about |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6284 +/- ##
==========================================
+ Coverage 70.14% 70.21% +0.06%
==========================================
Files 1840 1840
Lines 159115 159102 -13
Branches 33556 33556
==========================================
+ Hits 111619 111711 +92
+ Misses 33619 33512 -107
- Partials 13877 13879 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Fixes #6267.
Five follow-ups from #6221 / PR #6226. One correction to the issue first, because it changes what item 1 is:
DynamicMembershipTestdoes not share a cluster between its methods.beginTest/endTestare@BeforeEach/@AfterEach,serversis an instance field on a default-lifecycle JUnit class, and the Raft storage lives under the database directory thatbeginTestdeletes - so a removal never reaches the next method (verified: the ports rebind per method, and the class is green in isolation). Two of the three reported failures (:68,:116) are both theassertThat(leaderIndex).isGreaterThanOrEqualTo(0)line, i.e. the leaderless-window failure that #6226 already fixed by makingfindLeaderIndex()wait -d75174c92predates that merge. The third one is real, and it is a different bug: a method mutates the peer set that its own teardown then holds to a replica's contract.1. A withheld peer-to-peer endpoint now says so
getUnambiguousPeerHttpAddress/getUnambiguousPeerHttpsAddressrefuse an address two peers both resolve to by answeringnull(#6202), and every caller then decided for itself whether to log anything. Neither existing warning covers the refusal:deriveHttpAddressWithWarningand its HTTPS twin fire whenever an address is derived at all, which is also what a perfectly healthy homogeneous Kubernetes StatefulSet does, so they cannot distinguish "deriving, and fine" from "deriving, and two peers just collapsed onto one address";warnAmbiguousRoutingsays exactly the right thing but about the client routing tables of #6183.There is now a one-time WARNING per protocol, modelled on
warnAmbiguousRouting, naming the peers that could not be told apart, the address they share, and thehost:{raft:..,http:..}field to declare. HTTP and HTTPS get separate latches, for the reason #6221 already established: they are read from independent fields with independent derive fallbacks, so a cluster that declares distincthttpports and shares anhttpsone must still hear about the second.2. It is visible in cluster status, and the status endpoint was not the one the issue pointed at
The issue names
RaftClusterStatusExporter:85. That method -exportClusterStatus()- has no callers anywhere in the repo; the live status endpoint isGetClusterHandler, which assembles its own document. Decorating dead code would have changed nothing an operator sees, so:GET /api/v1/clusternow reports each peer'shttpAddressand, only when that address is not the peer's alone,httpAddressAmbiguous: true. The OpenAPI spec documents both (andPluginApiSpecTestpins the exact per-peer field set, so this could not be added silently). Studio renders the flag as a warning line on the node's card.exportClusterStatus()is removed rather than kept in step by hand. A second, unreachable builder of the same document is how two views of one cluster drift apart - and it had: the reachable one now reports the ambiguity and this one never would have.3. The presence matrix dialled the address nothing had checked
GET /api/v1/cluster?presence=trueasks every peer which databases it holds and attributes the answer to that peer, which makes it exactly the unattended dialPeerDialAddressexists to guard - the issue readGetClusterHandler:253as display, but it is a dial. Unguarded, on a cluster whose peers collapse onto one derived address, every peer was queried on the leader's own endpoint and reported the leader's database list as its own, so the matrix showed every database present on every node: the same false all-clear the verify endpoint gave before #6221.It resolves through
PeerDialAddressnow, so a peer it cannot identify goes intounreachable- with the refusal logged - instead of being answered for by whoever picked up. This is the better alternative to the flag the issue suggested for this call site: a flag next to a wrong answer is still a wrong answer.4.
RESYNC_RETRY_TIMEOUT_MS: 120 s -> 30 s, from the measurement#6226 added the instrument rather than guessing, and it has now reported. Across nine full
ha-integration-testsruns since that merge (31968696717, 31969178061, 31969810563, 31972218219, 31975575222, 31977924355, 31980155942, 31980224898 and the merge run itself), 235 tests each: not one wait exceeded the 10 s report threshold. The corroboration is the per-class wall clock - all ten classes that callwithResyncRetry/awaitValue/awaitCountOnran in every one of those runs, and the slowest took 53 s for the whole class, cluster startup and teardown included, so no single wait inside it can have approached even half the old budget.30 s is what the rest of
BaseRaftHATestalready treats as "long enough for the cluster to do anything it is going to do" -waitForReplicationIsCompleted,waitAllReplicasAreConnectedandLEADER_ELECTION_TIMEOUT_MSall use it. The budget with no measurement behind it was also the only one four times larger than its siblings; it is now one of them, still three times the largest wait the instrument can prove any of those runs needed, and a genuine hang costs 90 s less before it is reported.SLOW_WAIT_REPORT_MSdrops to 5 s with it: at 10 s a wait could consume a third of the new budget and still say nothing, which is the blindness that let the 120 s stand unmeasured for as long as it did.5. The membership test, and the redundant awaits
BaseRaftHATest.checkDatabasesAreIdentical()waits for, and compares, exactly the serversgetServerToCheck()names - the hook the base class already had for this - rather than every configured one. The two sets differ only for a test that takes a server out of the group: a peer that is no longer a member never applies another entry, so waiting for it can only burn 30 s and log a timeout, and comparing it can only report the divergence the eviction asked for.DynamicMembershipTestrecords what it evicted and excludes it. The class drops from 152 s to 77 s with one more test in it.Seven
await().until(() -> findLeaderIndex() >= 0)wrappers made redundant by #6226 are gone (RaftTimeSeriesReplication3NodesITx4 including its privateawaitLeaderElected(),SuperNodeAppendHAConsistencyIT,RaftTimeSeriesOversizedSealedIT,SuperNodeConcurrentAppendHABenchmark), and three verbatim copies of "only the servers still running" collapse intoBaseRaftHATest.startedServers().6. Docs
docs/release-26.9.1.mdcovers the operator-visible half. Thearcadedb-docsside is ArcadeData/arcadedb-docs#443: theretriesdefault (arcadedb.txRetries, with the run-more-than-once caveat for side effects outside the database), aVERIFICATION_INCOMPLETErow in a new "Verify Outcomes" table with the note for alerting that keys onINCONSISTENCY_DETECTED, and a "Peer-to-Peer Endpoints" section for the withheld-address behaviour above.Verification
New:
Issue6267AmbiguousPeerAddressWarningTest- 3 unit tests against a realRaftHAServerbuilt from a server list: the warning fires once across repeated asks, names both peers and the shared address; a correct cluster is silent; the HTTPS warning is not muted by the HTTP one.Issue6267AmbiguousAddressVisibilityIT- 3-node cluster: every peer flagged when the leader can identify none of them, no flag on the cluster's real addresses, and the presence matrix reporting the two peers unreachable with nothing attributed to them. Confirmed failing against the pre-fix dial (expected: 2unreachable, got 0).DynamicMembershipTest#removedPeerIsNotHeldToTheClusterConsistencyCheck- evicts a peer, writes through the remaining members, and asserts the evicted one does not see the write. Confirmed failing without thegetServerToCheckfix with exactly the reported shape:endTest > checkDatabasesAreIdentical: DatabaseAreNotIdentical Types: DB1 5 <> DB2 6, plus the 30 s "Timeout waiting for server 0 to replicate" (37.7 s vs 11.8 s with the fix).CapturingTestLoggergainedcountFormattedContaining(it captured only the raw template, which cannot assert which peers a warning named); the existing template-based assertions are untouched.Run locally: full
ha-raftunit lane (918 tests), the 18 IT classes these changes reach (35 tests) - the three whosegetServerToCheckwas collapsed, the four touched by the await cleanup, the verify guards, and all ten that use the shortened budget -server'sPluginApiSpecTest, and a full reactor build.