feat(#6091): the cluster can name a peer's gRPC address, so a follower's refusal is dialable - #6171
Conversation
…r's refusal is dialable A graph batch load refused on a follower named the leader's HTTP address and told the caller to work out the gRPC port itself, because HA knew a peer's Raft, HTTP and Bolt endpoints but not its gRPC one. It now names an address that can be dialled, and carries it on the trailers so a client redirects itself instead of parsing prose. - HAServerPlugin: BoltRoutingTable/getBoltRoutingTable generalised into RoutingTable(protocol, writer, readers) + getRoutingTable(ROUTING_PROTOCOL). One record and one resolver for both protocols rather than a second single-protocol copy inviting a third. - HA_SERVER_LIST object form takes a 'grpc:' field alongside 'bolt:'; when absent the address derives from the peer's Raft host plus this node's gRPC port, with the same one-time WARNING the Bolt fallback logs. The latch is now per protocol. - GlobalConfiguration.GRPC_PORT registers the key the gRPC plugin already read as a bare string, so HA and the plugin cannot disagree about the port (and it now honours the environment like every other setting). - The refusal carries the leader's gRPC and HTTP addresses on trailers and the ServerIsNotTheLeaderException class name, so the gRPC client rebuilds the same typed exception the HTTP protocol raises, with getLeaderAddress() pointing at the gRPC endpoint. Tests: Issue6091GrpcRoutingTableIT (declared addresses, protocol independence), Issue6091GrpcRoutingTableDerivedIT (fallback), Issue6091GraphBatchLoadLeaderRedirectIT (the refused load succeeds against the address the refusal named, dialled verbatim), Issue6091LeaderRedirectMappingTest (client-side typing). All mutation-checked.
|
Tick the box to add this pull request to the merge queue (same as
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 1 minor |
🟢 Metrics 0 complexity
Metric Results Complexity 0
🟢 Coverage 94.44% diff coverage
Metric Results Coverage variation Report missing for de774e31 Diff coverage ✅ 94.44% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (de774e3) Report Missing Report Missing Report Missing Head commit (e872854) 190025 137086 72.14% 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 (#6171) 72 68 94.44% 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%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
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.
ReviewRead through the full diff (bolt, engine, grpc, grpc-client, grpcw, ha-raft, server modules) and traced the cross-module wiring by hand (couldn't run OverallClean generalization of Things I specifically checked and are correct
Minor observations (non-blocking)
Test coverageCoverage looks thorough for the described scope: parsing ( No correctness, security, or performance issues found. Well-scoped, well-tested change. |
|
Codacy's one finding (Info, Declining it deliberately: the four enums already on |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6171 +/- ##
==========================================
+ Coverage 69.91% 69.93% +0.02%
==========================================
Files 1831 1831
Lines 157633 157682 +49
Branches 33253 33261 +8
==========================================
+ Hits 110208 110282 +74
+ Misses 33641 33600 -41
- Partials 13784 13800 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CI statusEverything this PR adds is green. The two red lanes are failures this branch did not introduce:
What this PR added, in the lanes that run it:
Re-running the two failed jobs to confirm. |
|
Re-run result for
So the only outstanding red in this lane is a known, separately tracked flake. |
|
Final CI state after re-running the two red lanes:
Codacy's single Info-level finding is the enum-naming one addressed above. |
1 participant
Closes #6091 (item 3 of #6083, the one left out of #6089).
The problem
graphBatchLoadrefuses a bulk load on a follower - correctly, since the bulk path mutates shared state only the leader can serialize (#4122). To let the caller redirect it named the leader, but the only address HA could expose was the leader's HTTP one, so the refusal had to say:A client retrying automatically had to already know the deployment's port-mapping convention, and there was no way to learn the leader's gRPC address from the cluster.
Decisions the issue asked to make deliberately
Generalise, don't duplicate.
BoltRoutingTable/getBoltRoutingTable()becomeRoutingTable(protocol, writer, readers)/getRoutingTable(ROUTING_PROTOCOL). A second single-protocol record would have invited a third, and the resolver behind it is the same code with different inputs: the declared-addresses map, this node's local port, and the one-time-warning latch are now selected per protocol, everything else is shared. The Bolt ROUTE path was updated with it;Bolt5002RoutingTableITpasses unchanged.Derive and declare, exactly as Bolt does. The object form of
arcadedb.ha.serverListtakes agrpc:field (host:{raft:2434,http:2480,bolt:7687,grpc:50051}); with nothing declared, a peer's endpoint is its Raft host plus this node's gRPC port. That is right for a homogeneous deployment (a K8s StatefulSet) and knowingly wrong for a heterogeneous one, which is why the field exists and why the fallback logs a WARNING naming the syntax. Shipping the derive-only half would have left the heterogeneous case silently wrong - the same reasonbolt:exists. The warning latch is per protocol now, so a derived Bolt address no longer mutes the gRPC warning.One RPC refuses today, but the refusal is not written for one RPC.
notTheLeader(ha, rpc, why)builds it, so the next RPC that has to refuse on a follower gets the same answer rather than a second dialect.Better than what the issue proposed
The issue asked for the address in the refusal message. A message is not a contract - a client redirecting itself would be pattern-matching prose. So the refusal also carries:
arcadedb-leader-grpc-address- the address the refused call can be retried on;arcadedb-leader-http-address- always known when a leader is, kept as the diagnostic fallback;arcadedb-exception-class: ServerIsNotTheLeaderException- the trailer this service already uses to preserve exception types.The gRPC client therefore rebuilds the same exception the HTTP protocol raises for the same situation,
ServerIsNotTheLeaderException, withgetLeaderAddress()returning the gRPC endpoint. Callers that already handle the HTTP leader redirect handle the gRPC one with no new code. The keys live in the sharedgrpcprotocol module (LeaderRedirectProtocol), next toGraphBatchProtocol, so the two ends cannot drift.GlobalConfiguration.GRPC_PORTregisters the key the gRPC plugin previously read as a bare string with its own literal default. HA resolves peer endpoints from the same constant, so the two cannot disagree about which port to advertise - and the setting now honours env/system properties like every other one.Testing
Cluster tests, because the follower branch is unreachable on a single server (recorded in #6083: the auth interceptor short-circuits a credential-less call and forcing the branch NPEs on a null
ha).Issue6091GraphBatchLoadLeaderRedirectIT(grpcw, 3 nodes, per-nodegrpc:declared) - the load refused on a follower is retried against the address the refusal advertised, dialled verbatim, and succeeds and replicates. A test comparing the string against the port it configured would pass just as well if the server had named the follower's own port, which is exactly the mistake to catch here.Issue6091GrpcRoutingTableIT(ha-raft) - every node names the same leader endpoint; the Bolt and gRPC tables disagree on a cluster that declares only gRPC ports (a shared resolver's one new failure mode is reading the other protocol's map); the writer is never also a reader.Issue6091GrpcRoutingTableDerivedIT(ha-raft) - the fallback, asserted against a deliberately non-default port so it fails if the resolver stops readingGRPC_PORT.Issue6091LeaderRedirectMappingTest(grpc-client) - gRPC address preferred over HTTP, HTTP as fallback, null address mid-election, and an address trailer alone does not invent a type.RaftHAServerAddressParsingTest-grpc:parsing, independence frombolt:, malformed port, K8s DNS suffix.Mutation-checked, all four ways: naming only the HTTP address, pointing the gRPC lookup at the Bolt map, deriving from
BOLT_PORT, and each fails the test that covers it.Suites run green:
boltunit +Bolt5002RoutingTableIT,ha-raftunit + the new ITs,grpcwunit +Issue6070*IT,grpc-clientunit. (Some local runs of unrelated classes fail onHTTP port 2480 not available- another process on this machine holds it.)Not in this PR
The user-facing
serverListreference lives in the docs repo; thegrpc:field wants the same one-paragraph notebolt:got there.