feat(server): add a storage-aware GET /readiness endpoint

[SebastianGruza]

Purpose of the PR

Closes #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 #3132: ready=true with 0 Stores for 150+ s while every GET /graph/vertices/<id> ended in a 500 after the 30 s request bound). Design discussed in the issue.

Main Changes

GET /readiness answers 200 while this Server can serve graph traffic and 503 otherwise, unauthenticated like /versions (whitelisted in AuthenticationFilter and PathFilter, so an httpGet probe needs neither a credential nor a graphspace prefix), with a JSON body that carries no addresses:

{"ready":true,"storage":"hstore","reason":"ok","active_stores":3,"answered_store":6386607161404282741,
 "pd_reachable":true,"pd_checked_age_ms":312,"stores_age_ms":312,"store_millis":3,"cached":false,
 "probes":[{"storage":"hstore","ready":true,"reason":"ok"}]}

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 reported pd_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; KnownStores holds the last PD answer) and a storage_readiness meta handler in HstoreStore.
• hugegraph-api: ReadinessAPI and StorageReadiness (every distinct remote backend configuration this Server uses is probed as the internal admin through HugeGraph.metadata(null, "storage_readiness", timeout): graphs sharing a configuration (same pd.peers, or same hbase.hosts and 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 one probes entry per configuration (backend and outcome, no graph name: the endpoint is unauthenticated); every hbase graph is its own scope, since the hbase provider derives the table namespace from the graph name, and the key carries the full connection settings (hosts, port, znode parent); every probe runs as the internal admin on its own thread and closes the graph transaction it opened; a configured graph that failed to load at startup (GraphManager logs and skips it) makes the Server not ready whatever its backend, with the count in failed_graphs and the failed configs' backends in failed_backends and in storage; the gate clears when a graph of that name is created through the API; the result is cached for readiness.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 with storage=embedded); the api module gains no dependency.
• ServerOptions: readiness.timeout (default 1000 ms), readiness.cache_ttl (default 2000 ms) and readiness.max_waiters (default 16: callers beyond it get an immediate 503 instead of waiting on the in-flight probe).
• hugegraph-hbase: HbaseStore answers storage_readiness with admin round trips within the budget: every table of every store of the graph (the schema store's labels, property keys and counters, the graph store's vertices, edges and indexes, the system store's meta) exists, is enabled and is available, so one disabled or unassigned table is not ready; the body's storage field names the probed backend.

Verifying these changes

• Add new unit test(s): 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 (14, including the two filter whitelists, concurrent callers sharing one probe, bounded follower wait, every remote configuration probed with one failing or hung configuration, the admin context on the async probes, the configuration keys, the failed-load body) and HbaseReadinessTest (all tables available, one of several disabled including a schema-store or counters table alone, failing and hung checks).
• ReadinessApiTest in the API suite on the memory, rocksdb, hbase and hstore CI jobs.
• E2E on a three-node k3s with the chart from feat(helm): add hstore-cluster k8s deployment chart #3218 / feat(helm): add HStore deployment chart hugegraph/hugegraph#221, 1 Hz sampling of /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 with pd_reachable=false; rolling restart of PD, zero transitions; SIGSTOP the busiest Store, zero transitions). Scripts, per-scenario JSON and the two superseded probe designs (an existsTable ping flapped on a roll; probing PD on every request tracked PD instead of storage): https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3212

Does this PR potentially affect the following parts?

• Introduce new configurations: readiness.timeout, readiness.cache_ttl, readiness.max_waiters in ServerOptions, documented in rest-server.properties and in the "Readiness endpoint" section of hugegraph-server/README.md
• New unauthenticated endpoint GET /readiness; no data-format change.

Chart side

A companion change adds server.readinessPath (default /versions) to the #3132 chart, mirroring pd.readinessPath; set it to /readiness on an image that serves it. Diff and measurements are in the validation repo above and will go to hugegraph#221.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 71.01124% with 129 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.57%. Comparing base (d00efce) to head (7f15a58).
⚠️ Report is 28 commits behind head on master.

Files with missing lines	Patch %	Lines
...graph/backend/store/hstore/HstoreStorageProbe.java	52.76%	61 Missing and 16 partials ⚠️
...apache/hugegraph/api/profile/StorageReadiness.java	82.88%	20 Missing and 12 partials ⚠️
...ache/hugegraph/backend/store/hbase/HbaseStore.java	80.35%	5 Missing and 6 partials ⚠️
...n/java/org/apache/hugegraph/core/GraphManager.java	50.00%	4 Missing ⚠️
...ph/backend/store/AbstractBackendStoreProvider.java	33.33%	1 Missing and 1 partial ⚠️
...e/hugegraph/backend/store/hbase/HbaseSessions.java	50.00%	0 Missing and 2 partials ⚠️
...org/apache/hugegraph/api/profile/ReadinessAPI.java	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #3221       +/-   ##
=============================================
+ Coverage     41.42%   52.57%   +11.14%     
+ Complexity     6931     2296     -4635     
=============================================
  Files           762      765        +3     
  Lines         66773    67844     +1071     
  Branches       8939     9195      +256     
=============================================
+ Hits          27662    35669     +8007     
+ Misses        35957    28600     -7357     
- Partials       3154     3575      +421     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bitflicker64]

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.

[bitflicker64]

⚠️ /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 closeStub after a watch error) with peers refusing fast: await() unwraps the PDException from newBlockingStub(), 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.47 DnsNameResolver), 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?

[SebastianGruza]

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.

[bitflicker64]

⚠️ Every cache-missed probe submits a new PD refresh here, even while the previous one is still running, and 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?

[SebastianGruza]

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.

[bitflicker64]

🧹 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.

[SebastianGruza]

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).

[bitflicker64]

🧹 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?

[SebastianGruza]

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]

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.

[bitflicker64]

⚠️ This ten-line anonymous 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.

[SebastianGruza]

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.

[bitflicker64]

🧹 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.

[SebastianGruza]

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.

[bitflicker64]

🧹 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.

[SebastianGruza]

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.

[SebastianGruza]

Round 1 in c9e70b1: reason carries a category only (gRPC status code, pd unreachable, class name) with the full messages in the log; the PD refresh is single-flight; channels of replaced Stores are shut down after a successful listing; Javadoc and option descriptions match the probe. Tests 18 + 6. On k3s with this build stores-zero and pd-zero PASS, and the sampled bodies contain 0 occurrences of host names or the PD port; results in results/issue-3212/round1-3212g of the validation repo.

[SebastianGruza]

Round 2 in 213fd9c: BasicThreadFactory instead of the hand-rolled factory, the graphName parameter and its concat removed, the body built directly as a map through one helper instead of the Result holder; tests on map entries, 18/18; net -48 lines. Smoke on k3s with this build: the three Server pods answer 200 with the same body as before the refactor, no readiness errors in the log.

[bitflicker64]

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.

[bitflicker64]

⚠️ 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.

[SebastianGruza]

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.

[bitflicker64]

🧹 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?

[SebastianGruza]

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.

[SebastianGruza]

Round 3 in 4a4aa1c: /readiness on the LoadDetectFilter whitelist (a busy Server still answers its probe from the storage state), an empty PD answer no longer closes the channels, plus ReadinessApiTest in the API suite (with and without credentials, on every backend of the suite) so the endpoint is measured by jacoco in the api-test job. Tests: LoadDetectFilterTest 11/11, HstoreStorageProbeTest 18/18, StorageReadinessTest 6/6, ReadinessApiTest 2/2 against a server built from this head on rocksdb.

[bitflicker64]

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]

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().

[imbajin]

⚠️ A successful getScanState response is treated as proof that this Server can serve graph traffic, but this RPC only reads local scan-pool statistics and never checks Raft or graph-data access. A Store whose status RPC remains healthy while its Raft or partition path is unavailable will still make /readiness return 200, so Kubernetes can keep routing graph requests to an unusable instance. Please validate the graph-serving path, or use a Store health signal that covers it, before reporting ready.

[SebastianGruza]

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:

1. 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, HgStoreNodePartitionerImpl on a NOT_WORK notice), 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.
2. 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-zero the load failed 2 of 244 requests with the ping gate and 387-560 of ~640 with any read gate; in pd-roll 1 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.

[imbajin]

⚠️ This synchronized method holds the class monitor while the Store probe performs network I/O for up to readiness.timeout. With the supported readiness.cache_ttl=0 setting, concurrent requests each trigger another probe and queue behind the previous call, so request latency can exceed the configured timeout by multiple probes and occupy REST workers. Please share an in-flight probe or bound each caller's wait without holding the monitor during network I/O.

[SebastianGruza]

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.

[SebastianGruza]

Round 4 in 517dfe6: StorageReadiness.check without synchronized, one in-flight probe shared by concurrent callers with a bounded wait (StorageReadinessTest 8/8). The gate on a read through the graph path was built in three variants and measured on k3s: in every one a rolling Store restart takes all Servers out of the Service while the traffic is fine, and with PD down a probe through the shared Store client raises the load failures from 2/244 to 58-87 %, because a failed read invalidates the partition cache the queries use; details in the inline reply and in results/issue-3212/round4-* of the validation repo. The gate stays on the ping, its scope is now stated in the Javadoc and in the description; the control battery is 7/7 PASS on this head.

[bitflicker64]

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.

[bitflicker64]

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?

[SebastianGruza]

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]

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.

[imbajin]

⚠️ This whitelist entry makes 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().

[SebastianGruza]

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).

[imbajin]

⚠️ 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().

[SebastianGruza]

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.

[SebastianGruza]

Round 5 in 7fa8020: the result and its timestamp in one holder, readiness.max_waiters with an immediate 503 for the excess, and a probe for graphs on HBase (HbaseStore.storage_readiness, one admin round trip within the budget). StorageReadinessTest 10/10.

[bitflicker64]

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.

[bitflicker64]

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.

[SebastianGruza]

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.

[imbajin]

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.

[imbajin]

[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.

[SebastianGruza]

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]

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().

[imbajin]

‼️ Table existence does not prove that an HBase graph table can serve traffic. This call uses HbaseSessions.existsTable(), which invokes Admin.tableExists; an existing disabled table is still reported as ready, while graph reads and writes fail. Please check that the selected table is enabled/available (or perform a bounded read through the serving path) before returning ready, and add a disabled-table case.

[SebastianGruza]

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).

[imbajin]

⚠️ The PD refresh prunes old-address channels before KnownStores.update() publishes the new list, while a concurrent probe can already have snapshotted known.stores() at line 250. During a full Store replacement, that probe still pings the old list and can return not-ready although PD has returned the replacement list; StorageReadiness then caches the false result for the configured TTL. Please make pruning and pings use one store-list generation, or defer channel shutdown until probes using the previous snapshot finish.

[SebastianGruza]

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).

[SebastianGruza]

Round 6 in 6cdd6ce (the branch is rebased onto master, hence the force push; the only conflict was UnitTestSuite): one probe per distinct remote backend configuration with a probes list in the body, HBase checks enabled plus available with a disabled-table test, deferred channel shutdown in the hstore probe, ReadinessApiTest accepts the hbase job. Unit tests green. The k3s cluster of the lab is switched off, so I did not rerun the round-4 battery; these changes do not touch the ping gate measured there. The PR body is updated with probes and the "every configuration" scope.

[imbajin]

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.

[imbajin]

‼️ Critical. Blocking: yes. Summary: With more than one remote configuration, probeAll dispatches metadata checks to CompletableFuture threads, but runAsAdmin installs AuthContext only on the request thread. With authentication enabled, HugeGraphAuthProxy.metadata then throws Missing authentication context and readiness returns 503 even when the backends are healthy. Please propagate the admin context into each async probe and cover an authenticated multi-configuration request. Evidence: AuthContext is a ThreadLocal; this branch uses supplyAsync while the one-configuration path is synchronous.

[SebastianGruza]

Right. Every probe on the pool runs inside HugeGraphAuthProxy.runAsAdmin() on its own thread. Test testAsyncProbesCarryTheAdminContext: three configurations, every probe sees the admin user's context.

[imbajin]

‼️ Critical. Blocking: yes. Summary: This checks only tableNames().get(0), while HbaseGraphStore registers separate vertex, edge and index tables. If the selected table is available while another graph-critical table is disabled or unassigned, readiness still returns 200 although operations using that table fail. Please check the graph-critical tables within the timeout or use a representative graph-serving operation, and add a case where a nonselected table is unavailable. Evidence: storageReadiness selects one table and tableAvailable checks only that table.

[SebastianGruza]

Right. The probe now checks every table of the graph (vertices, edges, indexes, counters) one after another within the budget: firstUnavailable(tables, check) returns the number of the first table that is not enabled and available, and the reason reads "table N of the graph is not available". Test: four tables with the third disabled is not ready; an empty table list is not ready; a failing admin call surfaces as a failure.

[imbajin]

⚠️ Important. Blocking: no. Summary: /readiness is unauthenticated, but each probes entry includes the graph name; failure reasons also include graph names. This exposes the graph inventory to callers without the graph-list permissions enforced elsewhere. Please omit graph names from the public response or move per-graph diagnostics behind an authenticated endpoint. Evidence: ReadinessAPI is @permitAll and probeEntry inserts r.name.

[SebastianGruza]

Right. The probes entries carry the backend and the outcome only (no graph name), and a failure reason reads "hbase configuration 2 of 2: ...". The unit test asserts the body contains no graph name, and ReadinessApiTest asserts there is no graph field.

[imbajin]

⚠️ Important. Blocking: no. Summary: On the first probe, await(refresh, deadline) can throw InterruptedException, but this catch treats it as an ordinary PD failure and returns without restoring the interrupt flag. Cancellation or shutdown can therefore be swallowed while waiting for the initial Store list. Please handle InterruptedException separately, restore the flag, and report interruption. Evidence: the Store-ping wait below restores the flag, but this branch catches Exception.

[SebastianGruza]

Right: a separate catch, the interrupt flag restored, and the reason "interrupted while waiting for the store list".

[bitflicker64]

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.

[bitflicker64]

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.

[SebastianGruza]

Right, thanks for the reproduction on memory. The probe is now probeOf(graph): metadata() in a try, graph.tx().close() in the finally when the transaction is open; this covers the request thread and the pool threads alike.

[SebastianGruza]

Round 7 in 067b796: the admin context on every async probe, the graph transaction closed after a probe, HBase checks every table of the graph, no graph names in the body, the interrupt flag restored on the first-probe wait. Unit tests green (StorageReadinessTest 12, HbaseReadinessTest, HstoreStorageProbeTest 18). The PR body is updated. The k3s cluster of the lab is still switched off, so the round-4 battery is not rerun.

[bitflicker64]

Blocking: yes. Summary: The round 7 fixes are in place (admin context on async probes, tx closed after a probe, every graph-store table checked, no graph names, interrupt flag restored). One gap is left: the HBase probe checks only the graph store's tables, so /readiness stays 200 while a schema or counters table is unavailable. Score 7/10. Evidence: static trace from StorageReadiness.probeOf through StandardHugeGraph.metadata -> graphTransaction() -> AbstractBackendStore.metadata to HbaseGraphStore.tableNames(), compared with HbaseSchemaStore.tableNames(). Latest-head CI: rocksdb-compatibility fails with ENOENT after/.github/workflows/.java-version because the branch predates #3264/#3261 on master. A rebase should clear it (git merge-tree against current master is clean).

[bitflicker64]

Important: This probe runs on the graph store only. graph.metadata(null, "storage_readiness", t) goes StandardHugeGraph.metadata -> graphTransaction().metadata -> AbstractBackendStore.metadata, so this is the HbaseGraphStore, and tableNames() lists vertices, edges and the index tables. The schema tables (vertex/edge label, property key, index label) and the counters table belong to HbaseSchemaStore; counters is added only in HbaseSchemaStore.tableNames(). The firstUnavailable Javadoc below lists counters, but this handler never checks that table. If a schema-table region is unassigned (a RegionServer crash, a region in transition) or the table is disabled, schema cache misses and schema id generation (SchemaTransaction -> store().nextId) fail while /readiness answers 200. That is the same false-ready case as the single-table finding in round 6, one store further. HbaseReadinessTest passes firstUnavailable a synthetic list, so it cannot catch this.

Requested change: check the schema store's tables as well, for example a second metadata call with a schema HugeType so it dispatches to the schema store, or pass both stores' table lists. Correct the Javadoc, and add a case where only a schema or counters table is unavailable.

[SebastianGruza]

Right, thanks for tracing the dispatch; I had read tableNames() as the graph's tables. The probe still runs on the graph store (that is where metadata() lands), but it now collects the tables of every opened store of the graph from the provider (AbstractBackendStoreProvider.openedStores()): the schema store first (labels, property keys, counters), then the graph store and the system store (meta). Every store shares the graph's namespace, so the graph store's sessions check them all. The firstUnavailable Javadoc now lists the actual tables, and the unit test has the case where only the counters table, or only a schema table, is unavailable (the number reported is theirs). The wiring itself (the stores a provider holds) has no unit test without an HBase cluster; it runs on the CI hbase job through ReadinessApiTest.

[imbajin]

Blocking: yes. Summary: Two source blockers remain in endpoint graph selection and HBase probe deduplication; a documentation gap is also noted. Evidence: current-head code-path review and repository guidance.

[imbajin]

‼️ Blocking: yes. Summary: The HBase key uses hbase.namespace, which the provider does not use because it derives table namespaces from graph names, and omits hbase.port/hbase.znode_parent; graphs on the same hosts or distinct ZooKeeper clusters can collapse to the first probe, leaving /readiness green when a skipped graph is down. Please key by the effective table namespace and full connection settings or probe every graph scope, with multi-graph coverage. Evidence: HbaseStoreProvider.namespace() derives the namespace from the graph name; HbaseSessions.open() uses hosts, port, and znode parent.

[SebastianGruza]

Right. The HBase provider derives the namespace from the graph name, so two HBase graphs never share a scope: the key is now the backend, the full connection settings (hbase.hosts, hbase.port, hbase.znode_parent) and the graph name, which means every HBase graph is probed on its own, and the body's probes list has one entry per graph. hstore keeps pd.peers as the key, since the probe pings the Stores that PD lists. configKey is static and testConfigKeys covers: two hbase graphs on one cluster get different keys, the same graph on two ZooKeeper parents gets different keys, two hstore graphs on one PD share one.

[imbajin]

‼️ Blocking: yes. Summary: This returns ready/embedded whenever no loaded remote graph is found, but GraphManager.loadGraphsFromLocal() logs and skips failed local graph loads; a configured HBase/HStore graph can disappear from manager.graphs() and still yield 200 even though that graph cannot serve traffic. Please distinguish a genuinely empty/no-remote configuration from graph-load failures before marking ready. Evidence: GraphManager.graphs() exposes loaded keys only and loadGraphsFromLocal() catches Throwable.

[SebastianGruza]

Right. GraphManager now records the configured graphs whose load failed (failedGraphs(), name to exception class, cleared when a later load of that graph succeeds), and StorageReadiness.check() answers not ready with "N configured graph(s) failed to load" and failed_graphs: N before looking at the remote backends, whatever the failed graph's backend was; no graph names in the body. testFailedGraphLoadIsNotReady covers the body; the empty-configuration case stays "embedded".

[imbajin]

🧹 Blocking: no. Summary: This adds the unauthenticated /readiness endpoint and three readiness options, but no operator documentation describes the endpoint contract or settings. Please document its status meaning and the timeout, cache TTL, and waiter options. Evidence: server AGENTS.md requires matching docs for user-visible features and configuration behavior; this PR changes source and tests only.

[SebastianGruza]

Done: a "Readiness endpoint" section in hugegraph-server/README.md (status contract, body fields, what ready means per backend, the shared-configuration rule, the three options with defaults) and the three options as commented entries in conf/rest-server.properties.

[SebastianGruza]

Round 8 in 3932be6 (rebased onto master again for the .java-version the rocksdb-compatibility job needs, hence the force push): the HBase probe covers every store of the graph (schema with counters, graph, system), every HBase graph is probed as its own scope with the full connection settings in the key, graphs that failed to load at startup make the Server not ready, and the endpoint and its options are documented in the README and in rest-server.properties. Unit tests green (StorageReadinessTest 14, HbaseReadinessTest, HstoreStorageProbeTest 18). The PR body is updated. The lab's k3s cluster is still off; the round-4 battery is not rerun.

[bitflicker64]

Blocking: no. Summary: The round 8 changes read correctly at this head: the HBase probe covers every store of the graph, each HBase graph is its own probe scope, a failed local load answers 503, and the endpoint and its options are documented. Two minor points are left: a failed-load entry is never cleared once startup is over, and three Javadoc blocks still describe the round 5 HBase probe. Evidence: static read of the full diff at 3932be6 against d00efce, and of GraphManager at that head (failedGraphs is written only in loadGraphsFromLocal, which after init() is called only by reload() when graph.load_from_local_config=true). CI on this head is green; the job logs show HstoreStorageProbeTest 18/18 in the memory, rocksdb, hbase and hstore jobs and ApiTestSuite (with ReadinessApiTest) passing on all four. No local test run was needed. Not verified here: whether the sequential HBase check (three admin calls per table, about 30 tables per graph across the three stores) fits the default 1000 ms budget on a real cluster; the standalone HBase in CI passes.

[bitflicker64]

Minor: this is the only place an entry leaves failedGraphs, and after init() the method is called again only from reload() when graph.load_from_local_config=true (default false). So a graph that fails its startup load keeps the Server at 503 for the life of the process. In particular, creating that graph afterwards through the graphs API (createGraph -> createGraphLocal with usePD=false) registers it in graphs through the GRAPH_CREATE listener but does not touch failedGraphs: the graph is served and /readiness still answers 1 configured graph(s) failed to load.

Requested change: remove the name from failedGraphs when a graph of that name is loaded or created later (for example in createGraphLocal after the create succeeds), and say in the README that a failed startup load otherwise holds until the Server restarts.

[SebastianGruza]

Right. createGraphLocal() now removes the name from failedGraphs once the create succeeded, so a graph created through the API after a failed startup load clears the gate; the README says that otherwise the gate holds until the Server restarts.

[bitflicker64]

Minor: three Javadoc blocks still describe the round 5 design. This one says the HBase probe is "an admin call about one of the graph's tables"; the remoteGraphs Javadoc (lines 156-157) says graphs with "the same HBase hosts and namespace" share one probe, which contradicts the configKey Javadoc below it and the README; ReadinessAPI (lines 42-43) says HBase is ready "while the cluster answers an admin call". None of them mentions the failed-load gate. The code checks every table of every store, probes each HBase graph on its own, and answers 503 on a failed load.

Requested change: update the three blocks to match the README section added in this PR.

[SebastianGruza]

Done: the class Javadoc of StorageReadiness, the remoteGraphs Javadoc and the ReadinessAPI Javadoc now describe what the code does (every table of every store on hbase, one scope per hbase graph, the failed-load gate, no names in the body) and point to the README section.

[imbajin]

Blocking: yes. Summary: The exact-head readiness gap remains documented in an existing thread; this review adds a separate response-contract issue where failed remote graphs are reported as embedded storage. Evidence: static trace of the PD graph-load path and the readiness response construction at this head.

[imbajin]

⚠️ Blocking: no. Summary: When remote graph loads fail, this derives storage only from successfully loaded graphs; if all failed graphs are remote, the failed-load response incorrectly says storage: "embedded", and mixed failures omit their backend. Please retain backend information for failed configurations or return an explicit unknown value. Evidence: remoteGraphs(manager) contains only loaded graphs, while failedBody() returns this value unchanged; the README defines embedded as having no graph on remote storage.

[SebastianGruza]

Right. GraphManager records the backend named in the failed graph's config (unknown when the file cannot be read), and the failed-load body lists those backends in failed_backends and merges them into storage, so two failed hstore graphs read as storage: "hstore" and a mixed case as "hbase,hstore"; embedded is used only when nothing is remote and nothing failed. testFailedGraphLoadIsNotReady covers the all-remote, mixed and unreadable-config cases.

[SebastianGruza]

Round 9 in 7f15a58: a graph created through the graphs API clears its failed startup load, the failed-load body names the failed configs' backends (failed_backends, merged into storage), and the three Javadoc blocks match the README. Unit tests green (StorageReadinessTest 14, HbaseReadinessTest); the PR body is updated.

[bitflicker64]

Blocking: yes. Summary: The round 9 changes read correctly for a Server without PD, but the new clearing of a failed startup load exists only on the usePD=false path, so with PD a graph created or loaded after a failed local load is served while /readiness stays 503. This is the remainder of the round 8 comment on failedGraphs; it is marked blocking because, when hit, the endpoint reports a serving Server as not ready on the deployment mode it was written for. Score 7/10. Evidence: full diff at 7f15a58 (21 files) read with GraphManager and StorageReadiness.check at that head; ApplicationConfig always calls manager.init(), which calls loadGraphsFromLocal whatever graph.load_from_local_config is; helm/hugegraph/templates/server-deployment.yaml and travis/start-server.sh set usePD=true. No test was run locally. CI at this head is green except pd_store / pd, pd_store / store and pd_store / hstore, which were still pending when checked, so ReadinessApiTest on hstore is not confirmed for this head yet. The open thread on the getScanState scope is left to the maintainer and is not repeated here.

[bitflicker64]

Important: this clears a failed startup load only when the create goes through createGraphLocal, and createGraph calls that only inside if (!isPDEnabled()) (line 1495). With usePD=true a create continues to this.graphs.put(graphName, graph) (line 1613), and the lazy load from meta in graph(graphSpace, name) (lines 2243-2244) takes the same path. Neither touches failedGraphs: at this head the map is written only at lines 1272, 1274 and 1461.

This is the PD half of my earlier comment on this method, which named only the usePD=false path. With usePD=true (what the Helm chart and the hstore CI job set), init() still loads conf/graphs. If a file fails there and a graph of that name is later created through the graphs API or loaded from meta, the Server serves it while /readiness keeps answering 503 with 1 configured graph(s) failed to load, so a readiness probe pointed at /readiness holds the Pod out of the Service. The README section added here says the gate clears when "a graph of that name is created through the graphs API", which holds only without PD.

Requested change: clear the entry on the PD path too, for a graph in the DEFAULT space (where loadGraph puts local graphs), after the GRAPH_CREATE notify has succeeded, as line 1461 does here, and add a case for it. If the gate is meant to hold with PD, please say that in the README instead.

[imbajin]

Blocking: yes. Summary: PD-loaded graph failures, a confirmed empty PD Store list, and HBase graphspace deduplication can each leave readiness green without every configured remote graph being available. Evidence: exact-head path review through GraphManager, HstoreStorageProbe, StorageReadiness, and HBase provider namespace construction.

[imbajin]

‼️ Critical. Blocking: yes. Summary: This records failures only in loadGraphsFromLocal; PD metadata graphs use loadGraph(Map), whose catch logs and continues when start_ignore_single_graph_error is enabled without updating failedGraphs. If the only PD-configured remote graph fails to load, remoteGraphs() sees none and readiness returns ready=true with storage=embedded. Please track failed PD-loaded graphs too and clear them after a successful load. Evidence: the loadGraph(Map) catch at lines 1357–1362 and StorageReadiness.check() empty-remote branch.

[imbajin]

⚠️ Important. Blocking: yes. Summary: A successful empty PD response sets pdOk=true but this condition keeps the previous nonempty Store list. probe() then uses that stale list without waiting for the refresh and can return ready when an old Store still answers, even though PD reports zero active Stores. Please replace the cached list on a successful empty response and preserve stale entries only when refresh fails or times out. Evidence: KnownStores.update() and probe() branches at lines 258–279 and 316–320.

[imbajin]

⚠️ Important. Blocking: yes. Summary: graph.name() is only the store name, but the HBase provider derives its namespace from the graph-space-qualified identity. Two HBase graphs with the same name in different graphspaces and identical connection settings therefore collide in this key, so only one graph's tables are checked and readiness can be green while the other is unavailable. Please key by the full graph-space-qualified backend identity and cover this case. Evidence: StandardHugeGraph.name(), BackendProviderFactory composes graphSpace, slash, and store, and HbaseStoreProvider.namespace().

[imbajin]

⚠️ Blocking: no. Summary: This embedded response and failedBody() omit cached, while the new README contract says every JSON body carries whether the answer was cached. Please include cached=false on these non-cached response paths. Evidence: hugegraph-server/README.md readiness response contract; the remote-probe path sets this field in probeOnce() and notReady().

[imbajin]

⚠️ Blocking: no. Summary: This cached pool is unbounded, and the probe submits one blocking ping task for every known Store before waiting for the first answer. A large PD Store list can create and retain one worker per Store from a single readiness request; the API waiter cap does not limit this fan-out. Please bound ping concurrency or use asynchronous gRPC calls while preserving the shared deadline. Evidence: probe() submits all Stores to this executor at lines 288–294.

[imbajin]

⚠️ Blocking: no. Summary: A timed-out table check only calls cancel(true) on a task in this unbounded cached pool; cancellation does not guarantee that a blocking HBase Admin RPC has stopped, and the Admin stays open until the task exits. After the result cache expires, repeated probes can accumulate workers and Admin resources during a slow HBase outage. Please bound or single-flight outstanding HBase checks per graph and ensure the HBase operation timeout fits the readiness budget. Evidence: readinessOf() submits the blocking check at line 190 and returns on timeout at lines 196–198; tableAvailable() closes the Admin only after its calls return.