feat(server): add a storage-aware GET /readiness endpoint - #3221
SebastianGruza wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3221 +/- ##
============================================
+ Coverage 41.57% 41.63% +0.05%
- Complexity 7311 7349 +38
============================================
Files 793 796 +3
Lines 69106 69486 +380
Branches 9258 9301 +43
============================================
+ Hits 28730 28928 +198
- Misses 37098 37253 +155
- Partials 3278 3305 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the probe design (last known Store list, parallel pings, one shared budget) holds up and is well tested, but the unauthenticated endpoint's body can leak PD peer and Store host names through exception text, and a hung PD makes every cache-missed probe park another thread on an unbounded pool. Two doc and resource nits besides. Evidence: static read of the d0f4a25 diff against PDClient (getStub/newBlockingStub, PDConfig.grpcTimeOut=60000), HstoreSessionsImpl.initStoreNode and grpc-core 1.47 DnsNameResolver; CI green on this head apart from codecov, and HstoreStorageProbeTest (15) passes in the hstore job log.
| return System.currentTimeMillis() - since; | ||
| } | ||
|
|
||
| private static String message(Throwable e) { |
There was a problem hiding this comment.
/readiness is unauthenticated and the PR says the body carries no addresses, but reason embeds raw exception messages. Two paths:
- No Store list known yet and the PD client has no live stub (never connected, or reset by
closeStubafter a watch error) with peers refusing fast:await()unwraps thePDExceptionfromnewBlockingStub(), whose message is"PD unreachable, pd.peers=" + config.getServerHost()(PDClient.java:142-143), so every PD peer is listed. - Every known Store fails and one failed name resolution:
UNAVAILABLE: Unable to resolve host <host>(grpc-core 1.47DnsNameResolver), if Stores register by DNS name.
StorageReadiness.check also appends e.getMessage(); testMapCarriesNoAddresses covers only the ready path.
Could reason use a fixed category (pd unreachable, the gRPC status code or exception class) and log the full message instead?
There was a problem hiding this comment.
Done in c9e70b1, thanks, both paths were real: with no Store list yet the PDException from newBlockingStub() carried the whole pd.peers, and with every Store down gRPC carried Unable to resolve host … (it was in my own Stores→0 samples, I had not connected the dots). reason now carries a fixed category only: the gRPC status code for StatusRuntimeException, pd unreachable for PDException, the class name otherwise (HstoreStorageProbe.category()); the full messages go to the log (WARN for PD, DEBUG for Store pings). StorageReadiness.check() likewise: class name only, message to the log. testReasonCarriesNoPdPeersNorStoreHosts feeds a PDException with pd.peers=pd-0.internal:8686,… and an UNAVAILABLE: Unable to resolve host store-0…svc and asserts neither map contains internal, svc or 8686; StorageReadinessTest checks the same for a probe exception. On k3s with this build the reasons read none of 3 known store(s) answered: a store failed: UNAVAILABLE; a store failed: UNAVAILABLE; a store failed: UNAVAILABLE, and a grep of every sampled body from stores-zero and pd-zero for .svc, 8686 and hugegraph-store- finds 0 occurrences.
| // Refresh the store list from PD in the background; whatever PD | ||
| // answers lands in `known` for this or the next probe | ||
| CompletableFuture<List<Metapb.Store>> refresh = new CompletableFuture<>(); | ||
| executor.execute(() -> { |
There was a problem hiding this comment.
EXECUTOR is an unbounded cached pool. getActiveStores() is bounded only by the PD client deadline, 60 s by default (PDConfig.grpcTimeOut; HstoreSessionsImpl.initStoreNode does not override it).
If PD hangs instead of refusing connections, the probe still returns in milliseconds from the known Stores, so each probe parks one more thread on PD: up to about 30 at the default 2 s TTL, and one per request with readiness.cache_ttl=0, which the option allows, on an endpoint anyone can call. Without a live stub they also queue on the synchronized PDClient.newBlockingStub() alongside graph traffic.
Could the refresh be single-flight, e.g. keep the in-flight CompletableFuture and skip submitting until it completes?
There was a problem hiding this comment.
Done in c9e70b1. The refresh is single-flight: KnownStores keeps an AtomicReference to the in-flight CompletableFuture, refresh() returns the running one while it is not isDone() and only starts a new one after it finished (CAS on the reference, so two racing probes also share one). A hung PD therefore parks one thread per process regardless of readiness.cache_ttl, including 0. testRefreshIsSingleFlight: a lister sleeping 3 s, five probes in a row → one call, all five ready from the known list; after it completes the next probe starts the second. The queueing on the synchronized newBlockingStub() next to graph traffic is thereby bounded to that one thread as well.
|
|
||
| /** | ||
| * Storage-aware readiness for Kubernetes and load balancers: 200 while this | ||
| * server can serve graph traffic, 503 while PD or every Store is unreachable |
There was a problem hiding this comment.
🧹 This Javadoc says 503 while PD is unreachable. So do the StorageReadiness Javadoc ("PD answers and one active Store answers"), the HstoreStorageProbe class Javadoc ("a hung PD or Store turns into not ready") and the readiness.timeout description. The probe deliberately stays ready with PD down once a Store list is known (testKnownStoresKeepTheServerReadyWhilePdIsDown). Both option descriptions also say one Store call per probe, while it pings every known Store in parallel. Please reword these to match: PD only matters until the first Store list is known, and the Store cost is one call per known Store.
There was a problem hiding this comment.
Done in c9e70b1. The Javadoc of ReadinessAPI, StorageReadiness and HstoreStorageProbe and the descriptions of readiness.timeout and readiness.cache_ttl now say what the code does: PD only matters until the first Store list is known, afterwards it is refreshed in the background; the cost of a probe is one cheap call to every known Store in parallel, first answer wins; 503 only when none answers within the budget (or, before the first list, when PD does not answer).
| } | ||
|
|
||
| private static void pingScanState(Metapb.Store store, long timeoutMs) { | ||
| ManagedChannel channel = CHANNELS.computeIfAbsent(store.getAddress(), address -> { |
There was a problem hiding this comment.
🧹 CHANNELS keeps one plaintext channel per Store address and nothing ever removes or shuts one down. When a Store is replaced or comes back under a different address, the old channel stays allocated for the life of the process. Could channels whose address is no longer in the latest PD list be shut down after a successful refresh in probe?
There was a problem hiding this comment.
Done in c9e70b1. After every successful listing from PD, pruneChannels() shuts down (shutdownNow) and removes the channels of addresses no longer listed; a failed listing touches nothing. Wired into the lister lambda of probe(graphName, timeoutMs), so the pure probe(...) stays stateless. testChannelsOfReplacedStoresAreShutDown uses a small ManagedChannel subclass (the hstore module has no Mockito): the channel of a listed address stays open, the replaced one is shut down and removed, a null listing removes nothing.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the design is measured, justified and tested — the simplifications left are local: the probe hand-rolls a ThreadFactory that commons-lang3's BasicThreadFactory (the repo's own ExecutorUtil pattern) does in one expression, carries a graphName parameter nothing reads, and wraps its result in a 55-line Result holder whose only production consumer is toMap(). Evidence: reviewed the full diff at head d0f4a25 (10 files, +965/-0) with the head checked out; grep -n graphName HstoreStorageProbe.java shows the parameter declared at line 196 and never read; grep -rn HstoreStorageProbe.Result outside the file matches only the test; grep -rn BasicThreadFactory shows four uses in hugegraph-common's ExecutorUtil and commons-lang3 already imported in the hstore module. The endpoint itself, KnownStores, the background PD refresh, the first-answer-wins loop and the api/hstore split via the meta handler are all justified in the PR description (two superseded designs measured, seven E2E scenarios) and are not raised.
|
|
||
| public static final String META_STORAGE_READINESS = "storage_readiness"; | ||
|
|
||
| private static final ExecutorService EXECUTOR = Executors.newCachedThreadPool( |
There was a problem hiding this comment.
ThreadFactory with its own AtomicInteger is the thing BasicThreadFactory.Builder exists for — and this repo already uses it exactly this way, four times, in hugegraph-common's ExecutorUtil. commons-lang3 is already imported elsewhere in this module (HstoreSessionsImpl, HstoreTable), so no new dependency.
Requested change:
private static final ExecutorService EXECUTOR = Executors.newCachedThreadPool(
new BasicThreadFactory.Builder().namingPattern("storage-readiness-%d")
.daemon(true).build());Same names, same daemon flag, eight lines and one import (java.util.concurrent.ThreadFactory + AtomicInteger) gone.
There was a problem hiding this comment.
Done in 213fd9c, exactly as proposed: new BasicThreadFactory.Builder().namingPattern("storage-readiness-%d").daemon(true).build(), like ExecutorUtil. The anonymous factory, the AtomicInteger and two imports are gone; same thread names, same daemon flag.
| * @param graphName the store-side graph name, kept for the meta handler | ||
| * @param timeoutMs the whole budget for PD plus stores | ||
| */ | ||
| public static Map<String, Object> probe(String graphName, long timeoutMs) { |
There was a problem hiding this comment.
🧹 graphName is never read in this method — the javadoc even says "kept for the meta handler", which is scaffolding for a use that doesn't exist yet. The call site in HstoreStore builds this.namespace + "/" + this.store just to feed it.
Requested change: drop the parameter here and the concat at the call site. Re-add it the day something reads it — pd.getActiveStores(graphName) exists if a per-graph store list is ever wanted.
There was a problem hiding this comment.
Done in 213fd9c. probe(long timeoutMs) without the parameter, and the namespace + "/" + store concat in HstoreStore is gone with it. If a per-graph store list is ever wanted, pd.getActiveStores(graphName) comes back together with the parameter.
| } | ||
| } | ||
|
|
||
| public static final class Result { |
There was a problem hiding this comment.
🧹 The only production consumer of Result is probe(String, long) calling .toMap() on it immediately; outside this file the class appears only in HstoreStorageProbeTest. So these ~55 lines — eight final fields, an eight-positional-arg constructor (new Result(false, "...", 0, null, false, -1L, -1L, 0L) is hard to read at the four construction sites), five getters — exist to make test assertions prettier.
Requested change: have probe(...) build the LinkedHashMap directly (one small private result(boolean ready, String reason, ...) helper covers the four return points) and let the tests assert on map entries, the way testMapCarriesNoAddresses already does. Deletes the class and the .toMap() hops.
There was a problem hiding this comment.
Done in 213fd9c. Result is gone: probe(...) returns the Map<String, Object> built by one private result(ready, reason, activeStores, answeredStore, pdReachable, pdAgeMs, storesAgeMs, storeMillis) helper at the four return points, and StorageReadiness.check() gets the map directly (no more toMap() hops). The tests assert on map entries the way testMapCarriesNoAddresses already did; 18/18.
Add server.readinessPath (default /versions, so nothing changes on current images), mirroring pd.readinessPath: values.yaml, the schema (pattern ^/) and the Server readinessProbe in server-deployment.yaml, plus a README parameter row and a two-case unit suite (74 total). On a Server image that serves GET /readiness (apache#3212, proposed in apache#3221), setting the value to /readiness makes a Server that cannot serve graph traffic answer 503 and drop out of the Service instead of returning 500 to every graph request. Startup and liveness stay on /versions so a Server that merely lost its storage is not restarted. Implements the change proposed and measured by @SebastianGruza in #229 (six fault scenarios at 1 Hz sampling, zero readiness transitions across Store and PD rolling restarts).
… PD refresh, prune dead Store channels Review round 1 of apache#3221: - the reason field carries a fixed category (gRPC status code, 'pd unreachable' or the exception class) instead of raw exception text, which could name PD peers and Store hosts on an unauthenticated endpoint; the full messages go to the log - the background PD refresh is single-flight (KnownStores keeps the future in flight), so a hung PD parks one thread, not one per cache-missed probe - channels of addresses PD no longer lists are shut down after a successful listing - Javadoc and option descriptions say what the probe does: PD only until the first Store list is known, one call per known Store in parallel Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 1 in c9e70b1: |
…used graphName, map result) Review round 2 of apache#3221: the executor uses commons-lang3's BasicThreadFactory like ExecutorUtil does, the unread graphName parameter and its concat at the HstoreStore call site are gone, and probe() builds the body map through one private helper instead of a Result holder whose only consumer was toMap(); the tests assert on map entries. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 2 in 213fd9c: |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the earlier rounds are addressed at 213fd9c and the probe logic holds up. The main gap is that /readiness is not exempt from LoadDetectFilter the way /versions is, so a busy Server fails the probe; separately, an empty PD answer closes the Store channels the probe keeps pinging. Evidence: static read of the full diff at 213fd9c against LoadDetectFilter.WHITE_API_LIST, ServerOptions.MAX_WORKER_THREADS (default 2 * CPUS), KnownStores.update and pruneChannels; gh pr checks green on this head apart from the two codecov statuses.
| private static final AntPathMatcher MATCHER = new AntPathMatcher(); | ||
| private static final Set<String> FIXED_WHITE_API_SET = ImmutableSet.of( | ||
| "versions", | ||
| "readiness", |
There was a problem hiding this comment.
readiness is added to the auth and path whitelists but not to LoadDetectFilter.WHITE_API_LIST ("", apis, metrics, versions). So /readiness goes through the worker-load and free-memory checks, before the result cache is reached, and gets a 503 once max_worker_threads - 1 other requests are in flight. restserver.max_worker_threads defaults to 2 * CPUS, so on a 2-CPU pod the probe fails while 3 other requests run.
Under sustained load Kubernetes then drops busy Servers from the Service and shifts their traffic to the rest, so a deployment that moves its readiness probe from /versions to /readiness can lose every endpoint during a spike while storage is fine.
Could readiness go into WHITE_API_LIST next to versions (LoadReleaseFilter reads the same list, so the counter stays balanced), with a case like testFilter_WhiteListPathIgnored? If shedding load through readiness is intended, please say so in the ReadinessAPI Javadoc.
There was a problem hiding this comment.
Done in 4a4aa1c, thanks, that would have been a nasty interaction: a readiness probe shed under load would pull exactly the busiest Servers out of the Service while the storage is healthy. readiness is in LoadDetectFilter.WHITE_API_LIST next to versions; LoadReleaseFilter reads the same list, so the workLoad counter stays balanced. testFilter_ReadinessIgnoredLikeVersions in LoadDetectFilterTest: with a 2-thread limit and one request in flight the filter lets /readiness through without touching the counter and without a log entry, in the same shape as testFilter_WhiteListPathIgnored. Readiness is not meant to shed load; the ReadinessAPI Javadoc says it answers from the storage state.
| /** Shut down the channels of addresses PD no longer lists (replaced Stores). */ | ||
| static void pruneChannels(Map<String, ManagedChannel> channels, | ||
| List<Metapb.Store> stores) { | ||
| if (stores == null) { |
There was a problem hiding this comment.
🧹 Follow-up to the channel pruning from round 1. An empty PD answer shuts down every channel here, while KnownStores.update ignores an empty answer and keeps the old list (testEmptyPdAnswerIsNotReadyAndKeepsTheOldList). As long as PD answers with no active Store, each refresh closes the channels to the Stores that the pings still use, and the next probe opens new ones. probe() starts the refresh and pings right away, so a ping in flight when shutdownNow() runs fails with UNAVAILABLE, and the old-list rule that keeps the Server ready can still turn a probe into a 503.
Could this return early on an empty list as well (if (stores == null || stores.isEmpty())), the same rule update applies, and could testChannelsOfReplacedStoresAreShutDown check that an empty listing keeps the channels?
There was a problem hiding this comment.
Done in 4a4aa1c. pruneChannels() returns on an empty list as well (stores == null || stores.isEmpty()), the same rule KnownStores.update() applies: as long as the pings use the last known list, their channels stay open. testChannelsOfReplacedStoresAreShutDown now also checks that an empty listing neither shuts down nor removes the channel of a known Store. 18/18.
…an empty PD answer, add ReadinessApiTest Review round 3 of apache#3221: - readiness joins LoadDetectFilter.WHITE_API_LIST next to versions, so a busy server still answers its readiness probe from the storage state instead of shedding it (LoadReleaseFilter reads the same list); test case in LoadDetectFilterTest - pruneChannels() ignores an empty PD answer, the same rule KnownStores.update() applies, so the channels the pings still use stay open while PD reports no active Store; the channel test covers it - ReadinessApiTest drives GET /readiness through the API suite, with and without credentials, on every backend of the suite Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Round 3 in 4a4aa1c: |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: both findings from the last round are fixed at 4a4aa1c. readiness is in LoadDetectFilter.WHITE_API_LIST, and LoadReleaseFilter skips it through the same isWhiteAPI, so the work-load counter stays balanced. pruneChannels now returns on an empty PD answer, the same rule KnownStores.update applies. The fixes from the first two rounds are still in place, and I found nothing new in the rest of the diff. Evidence: read the full diff at 4a4aa1c (14 files, +1204/-1) and the 213fd9c..4a4aa1c delta, plus the call sites in LoadReleaseFilter, GraphManager.graphs(), HugeGraphAuthProxy.runAsAdmin and HstoreStore.registerMetaHandlers. gh pr checks is green on this head, codecov included. The hstore job log shows HstoreStorageProbeTest 18/18, and its API suite grew from 155 to 157 tests (the two in ReadinessApiTest) with no failures.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The readiness signal can stay green while graph-serving operations are unavailable, and concurrent probes can wait beyond their configured timeout when caching is disabled. Evidence: static trace of HstoreStorageProbe.getScanState() and StorageReadiness.check().
| }); | ||
| HgStoreStateGrpc.newBlockingStub(channel) | ||
| .withDeadlineAfter(timeoutMs, TimeUnit.MILLISECONDS) | ||
| .getScanState(SubStateReq.getDefaultInstance()); |
There was a problem hiding this comment.
There was a problem hiding this comment.
The diagnosis is right: getScanState touches neither raft nor the data. So I built the gate you suggest and measured it on the k3s cluster from the PR description (3 PD + 3 Store + 3 Server, the same seven scenarios, 1 Hz samples per Server pod, load through the Service), in three variants: a get of a sentinel key on the vertex table through the same Store client the queries use, (a) in one shared partition, (b) in a partition led by the Store that answered the ping (leader codes from PD's partition cache at each refresh), (c) in parallel, one partition per known Store, first success wins. Runs and samples: https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3212 (round4-graphread-3212{j,k,l}, control round4-final-3212m), write-up in docs/server-readiness.md there.
| gate | store-one-down | store-roll | store-freeze | pd-zero (load failed) | pd-roll (load failed) |
|---|---|---|---|---|---|
| (a) shared partition | PASS | 503 on 3/3 Servers at once, 8-35 s | 503 on 3/3, 4 s | 503 whole outage (87 %) | 503 on 3/3, 28-36 s (7 %) |
| (b) partition of the answering Store | PASS | 503 on 3/3, 23-42 s | 503 on 3/3, 20-28 s | 503 (58 %) | PASS (0 %) |
| (c) parallel, first success | 503 on 3/3, 3-12 s ×4 | 503 on 3/3, 13-35 s | 503 on 3/3, 3-7 s | 503 (62 %) | 503 on 3/3, 53-85 s (47 %) |
| ping (round 3) + this round's single-flight | PASS | PASS | PASS | PASS (2 / 244) | PASS (1 / 370) |
Two effects, in every variant:
- The reads do not fail independently. Every 503 reason is
... partition read(s) did not finish within 1000 ms, also in (c) while two of three Stores are healthy and their partitions are being read. On the first error the Store client invalidates its partition cache and sleeps 1 s before retrying (NodeTxExecutor.retryingInvoke,HgStoreNodePartitionerImplon aNOT_WORKnotice), so one moving partition stalls every read through that client past the budget. During a Store roll all three Servers went 503 together and were pulled out of the Service, while the load through the Service had 2-3 failures in ~45 requests. - A read through the query client is not a passive measurement. A failed probe read invalidates the partition cache shared with the real queries, and with PD down there is nothing to rebuild it from: in
pd-zerothe load failed 2 of 244 requests with the ping gate and 387-560 of ~640 with any read gate; inpd-roll1 of 370 versus 106 of 226. A readiness probe that takes the data plane down during a PD outage is worse than one that cannot see raft.
So the gate stays on the ping, and I state its scope in the Javadoc and the PR description instead of implying more: ready means this Server knows a Store list and reaches at least one Store over gRPC; a Store whose status RPC answers while its raft path is broken is not detected. A Store-side signal that covers raft without going through the shared client does not exist today (HgStoreState.getPeers is in the proto, not implemented in the store node). If you want it, I can add a partition-status RPC with the raft leader to the Store as a separate PR and give the Server probe a choice of signal; I would rather not fold a Store-module change into this PR.
| return holder.get(0); | ||
| } | ||
|
|
||
| public static synchronized Map<String, Object> check(Probe probe, long timeoutMs, |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right, done in 517dfe6: check() is no longer synchronized. The first caller without a valid cache becomes the owner (AtomicReference<CompletableFuture>) and runs the probe on its own thread; concurrent callers wait for that result, each at most its own readiness.timeout, then get 503 with a probe is still running after N ms; a shared result carries "shared": true. No monitor is held during I/O, so with readiness.cache_ttl=0 a slow Store no longer queues REST workers. StorageReadinessTest: four concurrent callers share one probe (gated by a CountDownLatch), and a follower's wait is bounded (200 ms while the probe is held for 5 s). On k3s: baseline p50 5 ms, p99 13 ms, cache ratio 0.62.
|
Round 4 in 517dfe6: |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the new in-flight sharing in StorageReadiness.check fixes the serialised probes from the last round, but it drops the monitor that kept lastResult and lastCheckedAt consistent, so a caller can get the previous probe's answer marked as a fresh cache hit. Evidence: static read of the full diff at 517dfe6 and the 4a4aa1c..517dfe6 delta (StorageReadiness.java, HstoreStorageProbe.java, StorageReadinessTest.java); no CI has run on this head because the branch conflicts with master in UnitTestSuite.java (git merge-tree against 2f827d6), so the new tests are unverified here.
| public static Map<String, Object> check(Probe probe, long timeoutMs, long cacheTtlMs) { | ||
| long now = System.currentTimeMillis(); | ||
| Map<String, Object> cached = lastResult; | ||
| if (cached != null && now - lastCheckedAt < cacheTtlMs) { |
There was a problem hiding this comment.
Minor: lastResult and lastCheckedAt are two separate volatiles, and since this commit nothing keeps them paired for a reader. The owner writes lastResult (line 111) and then lastCheckedAt (line 112). A caller that reads lastResult at line 99 just before line 111 runs, and lastCheckedAt at line 100 just after line 112, returns the previous probe's result with cached: true as if it were current, for example a 200 right after the new probe found no Store. In 4a4aa1c the synchronized method made the pair atomic. Could the result and its timestamp live in one immutable holder behind a single volatile or AtomicReference, so the TTL check and the returned body always come from the same probe?
There was a problem hiding this comment.
Right, thanks, I dropped that pairing together with the monitor in round 4. The result and its timestamp now live in one immutable Cached holder behind a single AtomicReference, so the TTL check and the returned body always come from the same probe.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The unauthenticated readiness endpoint bypasses load admission while callers wait on storage, and HBase graphs are reported ready without a remote-storage check. Evidence: exact-head static trace through the filter, readiness implementation, and HBase backend initialization.
| "metrics", | ||
| "versions" | ||
| "versions", | ||
| "readiness" |
There was a problem hiding this comment.
LoadDetectFilter.filter() return before it increments WorkLoad or enforces the worker-load and low-memory limits. /readiness is unauthenticated, and StorageReadiness.check() makes concurrent callers wait synchronously on one in-flight probe for up to readiness.timeout (60 seconds); during slow storage, a burst can occupy the REST worker pool and starve graph requests. Please cap concurrent readiness waiters and reject excess calls quickly while preserving one probe under load. Evidence: exact-head static trace through LoadDetectFilter.filter() and StorageReadiness.check().
There was a problem hiding this comment.
Right. New option readiness.max_waiters (default 16, range 1..10000): at most that many callers wait at once for the probe in flight, each still bounded by its own readiness.timeout; the rest get an immediate 503 with the reason too many readiness callers waiting for the probe (N) and hold no worker. One probe under load stays. Test testExcessWaitersAreRejectedAtOnce (cap 1: the owner, one waiter, a third caller rejected in under 500 ms, the slot free again afterwards).
| for (String name : manager.graphs()) { | ||
| try { | ||
| HugeGraph graph = manager.graph(name); | ||
| if (graph != null && BACKEND_HSTORE.equals(graph.backend())) { |
There was a problem hiding this comment.
firstHstoreGraph() selects only hstore; any HBase graph falls through to the caller's ready=true / storage=embedded branch. HBase remains an allowed, packaged backend, and HbaseStore.open() creates its connection without proving the cluster is reachable, so requests can fail while this probe stays green. Please handle supported remote backends explicitly and probe HBase or report not-ready/unknown when its availability is not checked. Evidence: exact-head static trace plus BackendProviderFactory.ALLOWED_BACKENDS, the dist POM, and HbaseStore.open().
There was a problem hiding this comment.
Right. firstRemoteGraph picks the first graph on a remote backend (hstore or hbase), and HbaseStore registers a storage_readiness meta handler: one admin round trip (existsTable of the graph's first table) on its own executor within the readiness.timeout budget; the body carries ready/reason/hbase_millis, no addresses and no raw exception text, and its storage field names the probed backend. I have no HBase cluster in the lab; the path runs in this PR's api-test job on hbase through ReadinessApiTest, and the probe itself is the same executor + Future.get(budget) shape as the hstore one.
|
Round 5 in 7fa8020: the result and its timestamp in one holder, |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Round 5 makes a server with an HBase graph report storage "hbase", but ReadinessApiTest still only accepts "embedded" or "hstore", so testReadyOnAHealthyServer fails in the hbase API job that server-ci runs. The rest of the round 5 delta (the Cached holder, readiness.max_waiters, the HbaseStore probe) reads correctly. Evidence: static read of the full diff at 7fa8020 and the 517dfe6..7fa8020 delta. HbaseStoreProvider.type() returns "hbase", StandardHugeGraph.backend() returns that value, StorageReadiness.firstRemoteGraph() now selects it, and probeOnce() puts it in the body as "storage". server-ci.yml runs run-api-test.sh with BACKEND in [memory, rocksdb, hbase], and ApiTestSuite includes ReadinessApiTest. No CI has run on this head because the branch conflicts with master (mergeStateStatus DIRTY), so the failure is not visible yet.
| Assert.assertEquals(true, body.get("ready")); | ||
| Assert.assertTrue(String.valueOf(body.get("storage")), | ||
| "embedded".equals(body.get("storage")) || | ||
| "hstore".equals(body.get("storage"))); |
There was a problem hiding this comment.
Important: This assertion fails on the hbase API job after this round. StorageReadiness.firstRemoteGraph() now picks a graph whose backend is hbase, and probeOnce() writes graph.backend() into the body as storage. HbaseStoreProvider.type() returns "hbase", so the body on that job is {"ready":true,"storage":"hbase",...} and assertTrue fails with message hbase. The storage field alone decides this, so the test fails even when the HBase probe itself succeeds.
server-ci.yml runs run-api-test.sh for memory, rocksdb and hbase, and ApiTestSuite includes ReadinessApiTest. The round 5 reply says the HBase path is covered by this test on that job, but as written the job goes red instead. No CI has run on 7fa8020 yet because the branch conflicts with master, so this has not shown up.
Requested change: accept "hbase" here and check the HBase fields the same way as the hstore ones, for example reason is "ok" and hbase_millis is present. Please also update the class Javadoc, which still lists only embedded and hstore.
There was a problem hiding this comment.
Right, thanks, I did not think it through for the hbase job. The test accepts storage embedded, hstore or hbase; for hbase it checks reason == "ok" and the presence of hbase_millis, for both remote backends the presence of the probes list. The class Javadoc lists the three jobs.
There was a problem hiding this comment.
Fix required: update the HBase API assertion, resolve the branch conflict, and add endpoint/configuration documentation.
Define the multi-backend coverage boundary. Only the first remote graph is probed, so one healthy HBase cluster can hide a failed independently configured cluster.
| for (String name : manager.graphs()) { | ||
| try { | ||
| HugeGraph graph = manager.graph(name); | ||
| if (graph != null && REMOTE_BACKENDS.contains(graph.backend())) { |
There was a problem hiding this comment.
[P2] Define readiness for independently configured remote backends
With two graphs backed by independent HBase clusters, this returns the first graph permanently: a healthy first cluster makes /readiness return ready=true while a failed second cluster is never probed.
A unit harness using the exact StorageReadiness source produced: healthy graph metadata called once, failed graph metadata never called, result ready=true. This is a unit-level reproduction, not cluster acceptance.
Either probe each independent backend configuration within the common budget, or document that readiness covers only the selected first remote graph and does not certify other HBase/HStore configurations. The currently documented HStore status-ping limitation is a separate, accepted boundary.
There was a problem hiding this comment.
Right. Readiness now covers every distinct remote backend configuration: graphs with the same configuration (the same pd.peers, or the same hbase.hosts and namespace) share one probe, independent clusters are probed side by side within the common budget, and ready means all of them are ready. The body keeps the fields of the first probe, storage lists the backends, and a probes list carries one entry per configuration (graph, backend, ready, reason); on a failure reason names the configuration. Test testEveryRemoteConfigurationIsProbed: a healthy plus a failed configuration, a hung configuration bounded by the budget, a single configuration with the body unchanged.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The HBase probe can report ready for an existing but disabled table, and the PD refresh can produce a stale-list false negative. Evidence: static trace from HbaseStore.storageReadiness() to HbaseSessions.existsTable(), and from HstoreStorageProbe.probe() to KnownStores.refresh().
| String table = tables.isEmpty() ? null : tables.get(0); | ||
| long start = System.currentTimeMillis(); | ||
| Future<Boolean> exists = READINESS_EXECUTOR.submit(() -> { | ||
| return table != null && this.sessions.existsTable(table); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. New HbaseSessions.tableAvailable(table): tableExists && isTableEnabled && isTableAvailable through Admin. The probe logic moved to a static HbaseStore.readinessOf(Callable, timeout, executor), testable without a cluster; HbaseReadinessTest covers: available, existing but disabled (not ready, reason "the graph's first table is not available"), a failing admin call (no exception text in the body), and a hung call (bounded by the budget).
| } | ||
| return probe(KNOWN, () -> { | ||
| List<Metapb.Store> stores = pd.getActiveStores(); | ||
| pruneChannels(CHANNELS, stores); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Right. Channel shutdown is deferred: an address missing from a listing first goes into a stale set, its channel is shut down only when it is missing from two consecutive listings, and its return clears the state. A probe that snapshotted the previous list therefore still pings through open channels. testChannelsOfReplacedStoresAreShutDown rewritten for that scenario (the first listing keeps, a return forgets, two in a row shut down, an empty and a failed listing prune nothing).
Closes apache#3212. A Kubernetes readiness probe on /versions keeps a server in the Service while every graph request fails, because /versions answers 200 as long as the REST layer is up, even with no Store in the cluster (measured under after the 30 s request bound). GET /readiness answers 200 while this server can serve graph traffic and 503 otherwise, unauthenticated like /versions (whitelisted in both AuthenticationFilter and PathFilter, so an httpGet probe needs neither a credential nor a graphspace prefix), with a JSON body that carries no addresses. Ready means, from this server's own view: 1. its PD client answers (getActiveStores()), and 2. at least one Store that PD reports as active answers one cheap direct call (a node session and Table/EXISTS on the vertex table). The first Store that answers ends the probe: a rolling restart of the Stores never pulls the servers out of the Service, while zero Stores does. - hugegraph-hstore: HstoreStorageProbe (pure logic over an active-store lister and a store pinger, one shared time budget through an executor so a hung PD or Store yields "did not answer within N ms" instead of a hung probe) and a "storage_readiness" meta handler in HstoreStore - hugegraph-api: ReadinessAPI and StorageReadiness (first hstore graph is probed through HugeGraph.metadata(null, "storage_readiness", timeout), result cached for readiness.cache_ttl; servers without an hstore graph answer 200 with storage=embedded); the api module gains no dependency - ServerOptions: readiness.timeout (default 1000 ms) and readiness.cache_ttl (default 2000 ms) Tests: HstoreStorageProbeTest (9) and StorageReadinessTest (6, including the filter whitelists). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… PD refresh, prune dead Store channels Review round 1 of apache#3221: - the reason field carries a fixed category (gRPC status code, 'pd unreachable' or the exception class) instead of raw exception text, which could name PD peers and Store hosts on an unauthenticated endpoint; the full messages go to the log - the background PD refresh is single-flight (KnownStores keeps the future in flight), so a hung PD parks one thread, not one per cache-missed probe - channels of addresses PD no longer lists are shut down after a successful listing - Javadoc and option descriptions say what the probe does: PD only until the first Store list is known, one call per known Store in parallel Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…used graphName, map result) Review round 2 of apache#3221: the executor uses commons-lang3's BasicThreadFactory like ExecutorUtil does, the unread graphName parameter and its concat at the HstoreStore call site are gone, and probe() builds the body map through one private helper instead of a Result holder whose only consumer was toMap(); the tests assert on map entries. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…an empty PD answer, add ReadinessApiTest Review round 3 of apache#3221: - readiness joins LoadDetectFilter.WHITE_API_LIST next to versions, so a busy server still answers its readiness probe from the storage state instead of shedding it (LoadReleaseFilter reads the same list); test case in LoadDetectFilterTest - pruneChannels() ignores an empty PD answer, the same rule KnownStores.update() applies, so the channels the pings still use stay open while PD reports no active Store; the channel test covers it - ReadinessApiTest drives GET /readiness through the API suite, with and without credentials, on every backend of the suite Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ing callers on a monitor
StorageReadiness.check no longer holds the class monitor while the Store
probe does network I/O: the first caller runs the probe on its own thread,
concurrent callers wait for that result bounded by their own timeout
("a probe is still running after N ms"). With readiness.cache_ttl=0 a slow
Store can no longer queue REST workers behind it.
StorageReadinessTest 8: concurrent callers share one probe, the follower
wait is bounded.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…waiting callers, an HBase probe - StorageReadiness keeps the last result with its timestamp in one immutable holder behind a single AtomicReference, so the TTL check and the returned body always come from the same probe. - readiness.max_waiters (default 16): callers beyond it get an immediate 503 instead of waiting on the in-flight probe, so a burst of probes during slow storage cannot hold the REST worker pool (the endpoint is unauthenticated and outside the load-shedding filter). - Graphs on hbase are probed too: HbaseStore answers storage_readiness with one admin round trip (does the graph's first table exist) within the budget, on its own executor; the body's storage field names the probed backend. - StorageReadinessTest 10: excess waiters rejected at once, storage name follows the backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- probe one graph per distinct hstore (pd.peers) / hbase (hosts+namespace) configuration in parallel within the budget; ready = all, body lists probes - hbase: table enabled and available (not only existing), logic testable - hstore: channels of replaced stores are shut down only after two listings - ReadinessApiTest accepts the hbase CI job
7fa8020 to
6cdd6ce
Compare
|
Round 6 in 6cdd6ce (the branch is rebased onto master, hence the force push; the only conflict was |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The HBase probe can report ready while other graph tables are unavailable, and authenticated multi-configuration requests fail because admin context is not propagated to probe threads. Evidence: HbaseStore.storageReadiness checks one table; StorageReadiness.probeAll runs metadata calls with CompletableFuture.supplyAsync.
| long deadline = System.currentTimeMillis() + timeoutMs; | ||
| List<CompletableFuture<Map<String, Object>>> futures = new ArrayList<>(); | ||
| for (RemoteGraph r : remotes) { | ||
| futures.add(CompletableFuture.supplyAsync(() -> { |
There was a problem hiding this comment.
| */ | ||
| private Map<String, Object> storageReadiness(long timeoutMs) { | ||
| List<String> tables = this.tableNames(); | ||
| String table = tables.isEmpty() ? null : tables.get(0); |
There was a problem hiding this comment.
|
|
||
| private static Map<String, Object> probeEntry(RemoteGraph r, Map<String, Object> result) { | ||
| Map<String, Object> entry = new LinkedHashMap<>(); | ||
| entry.put("graph", r.name); |
There was a problem hiding this comment.
| } catch (TimeoutException e) { | ||
| return result(false, "no store list known and pd did not answer within " + | ||
| timeoutMs + " ms", 0, null, false, -1L, -1L, 0L); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the probe logic reads correctly at this head; one cleanup: the readiness probe opens a thread-local graph transaction on the request or probe thread and never closes it, so closing the graph later fails its all-threads-closed check. The two blocking findings imbajin raised on this head (admin context on the async path, single HBase table) still apply and are not repeated. Evidence: full diff at 6cdd6ce read against StandardHugeGraph.metadata/graphTransaction/close and BackendSessionPool; a standalone run on the memory backend reproduced the IllegalStateException from close(); CI green on this head.
| continue; | ||
| } | ||
| out.add(new RemoteGraph(name, graph.backend(), | ||
| t -> graph.metadata(null, STORAGE_READINESS_META, t))); |
There was a problem hiding this comment.
Minor: graph.metadata(...) here goes through StandardHugeGraph.metadata to graphTransaction(), which calls tx.readWrite() and auto-opens the thread-local transaction, and nothing closes it. With one remote configuration that is the REST worker thread; with several it is a storage-readiness-probe pool thread, which also opens new backend sessions whenever the cached pool replaces an idle thread. StandardHugeGraph.close() ends with E.checkState(this.tx.closed(), "Ensure tx closed in all threads ..."), so after any probe, closing this graph on shutdown or drop throws and gets logged as "Failed to close graph". VertexAPI and EdgeAPI close the transaction in a finally for this reason.
Evidence: against the hugegraph-core classes on the memory backend, calling g.metadata(null, "storage_readiness", 1000L) from a separate thread and then g.close() printed IllegalStateException: Ensure tx closed in all threads when closing graph 'DEFAULT-hugegraph'. The call itself raised NotSupportException on memory, so the transaction opens before the handler runs and hstore and hbase take the same path.
Requested change: close it after the probe, for example:
t -> {
try {
return graph.metadata(null, STORAGE_READINESS_META, t);
} finally {
if (graph.tx().isOpen()) {
graph.tx().close();
}
}
}and add a case where the graph closes cleanly after a probe.
3 participants
Purpose of the PR
Closes #3212. A Kubernetes readiness probe on
/versionskeeps a Server in the Service while every graph request fails, because/versionsanswers 200 as long as the REST layer is up, even with no Store in the cluster (measured under #3132:ready=truewith 0 Stores for 150+ s while everyGET /graph/vertices/<id>ended in a 500 after the 30 s request bound). Design discussed in the issue.Main Changes
GET /readinessanswers 200 while this Server can serve graph traffic and 503 otherwise, unauthenticated like/versions(whitelisted inAuthenticationFilterandPathFilter, so anhttpGetprobe needs neither a credential nor a graphspace prefix), with a JSON body that carries no addresses:Ready means, from this Server's own view: at least one Store from the last Store list PD answered with answers a direct, local, read-only gRPC call (
HgStoreState.getScanState, a read of the node's own scan-pool stats that never touches raft). The Store list is refreshed from PD in the background on every probe and never waited for once a list is known, so PD being down, slow or restarting only flips the reportedpd_reachable, never the readiness, as long as a Store answers. The pings run in parallel and the first answer wins, so a Store whose pod just left never eats the budget of the healthy ones.Scope: ready means this Server knows a Store list and reaches at least one Store over gRPC; a Store whose status RPC answers while its raft or partition path is broken is not detected. A gate on a read through the graph path was measured in three variants (review round 4) and rejected: a failed probe read invalidates the partition cache the queries share, which took the data plane down during a PD outage, and one moving partition stalled every read past the budget, which took every Server out of the Service during a rolling Store restart while the traffic was fine (
results/issue-3212/round4-*in the validation repo).hugegraph-hstore:HstoreStorageProbe(pure logic over a Store lister and a Store pinger, one shared time budget through an executor so a hung PD or Store yields "did not answer within N ms" instead of a hung probe;KnownStoresholds the last PD answer) and astorage_readinessmeta handler inHstoreStore.hugegraph-api:ReadinessAPIandStorageReadiness(every distinct remote backend configuration this Server uses is probed as the internal admin throughHugeGraph.metadata(null, "storage_readiness", timeout): graphs sharing a configuration (samepd.peers, or samehbase.hostsand namespace) share one probe, independent clusters are probed side by side within the common budget, ready means all of them are ready and the body lists oneprobesentry per configuration; the result is cached forreadiness.cache_ttl, one in-flight probe is shared by concurrent callers with a bounded wait, no monitor is held during I/O; Servers with no graph on a remote backend (hstore, hbase) answer 200 withstorage=embedded); the api module gains no dependency.ServerOptions:readiness.timeout(default 1000 ms),readiness.cache_ttl(default 2000 ms) andreadiness.max_waiters(default 16: callers beyond it get an immediate 503 instead of waiting on the in-flight probe).hugegraph-hbase:HbaseStoreanswersstorage_readinesswith one admin round trip within the budget (the graph's first table exists, is enabled and is available, so an existing but disabled table is not ready); the body'sstoragefield names the probed backend.Verifying these changes
HstoreStorageProbeTest(18: first answer wins, a hung Store does not hide an answering one, known Stores keep the Server ready while PD is down, a hung PD does not delay a probe with known Stores, the PD answer updates the known list, channels of replaced Stores are shut down only after two consecutive listings, budget bounding, no addresses in the body),StorageReadinessTest(11, including the two filter whitelists, concurrent callers sharing one probe, bounded follower wait, every remote configuration probed with one failing or hung configuration) andHbaseReadinessTest(available, disabled, failing and hung table checks).ReadinessApiTestin the API suite on the memory, rocksdb, hbase and hstore CI jobs./readiness,/versions, the pod Ready condition and the Service endpoints on every Server pod, load through the Service: 7 PASS (baseline; Stores→0 pulls every Server out of the Service in ~30 s and back in ~20 s; one Store deleted, no flap; rolling restart of the Stores under load, zero readiness transitions; PD→0 stays 200 withpd_reachable=false; rolling restart of PD, zero transitions; SIGSTOP the busiest Store, zero transitions). Scripts, per-scenario JSON and the two superseded probe designs (anexistsTableping flapped on a roll; probing PD on every request tracked PD instead of storage): https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3212Does this PR potentially affect the following parts?
readiness.timeout,readiness.cache_ttl,readiness.max_waitersinServerOptionsGET /readiness; no data-format change.Chart side
A companion change adds
server.readinessPath(default/versions) to the #3132 chart, mirroringpd.readinessPath; set it to/readinesson an image that serves it. Diff and measurements are in the validation repo above and will go to hugegraph#221.🤖 Generated with Claude Code