fix(server): wait for the stores at startup instead of exiting on a cold start - #3210
Conversation
…old start With usePD=true the first hstore graph opened at startup (a local conf/graphs graph, or the system graph created on the first boot) needs pd.initial-store-count active stores, and the store client gives up after a fixed 10 retries (about 38 s). Stores that register later than that make the server exit 1 on a cold start (apache#3203). GraphManager now polls PD for the active store count before any graph is opened, bounded by the new option pd.stores_wait_timeout (seconds, default 300, 0 keeps the old behaviour), logging the progress every 5 s and naming the option in the timeout error. close apache#3203
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3210 +/- ##
============================================
+ Coverage 37.90% 38.03% +0.13%
- Complexity 6600 6637 +37
============================================
Files 800 802 +2
Lines 69035 69266 +231
Branches 9186 9222 +36
============================================
+ Hits 26167 26348 +181
- Misses 39795 39840 +45
- Partials 3073 3078 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The startup wait needs a hard deadline for each PD RPC and a lifecycle-safe client. Evidence: current-head static tracing shows the new calls use PDClient's default 60-second gRPC deadline, and the temporary client starts watchers when its first blocking stub is created.
| pdConfig.setAuthority(PdMetaDriver.PDAuthConfig.service(), | ||
| PdMetaDriver.PDAuthConfig.token()); | ||
| // same short-lived client as limitStorage(); PDClient has no close() | ||
| PDClient pdClient = PDClient.create(pdConfig); |
There was a problem hiding this comment.
PDClient.create() is not a one-shot client: the first getPDConfig() call enters newBlockingStub(), which invokes startWatch() and opens the watch streams. This local client is discarded after the wait, while PDClient exposes no shutdown (closeStub() even notes that the managed channel is not closed), so every usePD startup leaves an unused watcher set for the process lifetime. Please use a closable one-shot stub or retain and close this client from the GraphManager lifecycle.
There was a problem hiding this comment.
Done in 3d64daa. Instead of a PDClient there is now a PdReadinessProbe: one plaintext gRPC channel per PD peer, a raw PDGrpc.newBlockingStub with the same Authentication interceptor that AbstractClient.setBlockingParams() adds (credentials from PDConfig.setAuthority(), so without PD authentication no interceptor is attached), no watchers, closed with shutdownNow() in a try-with-resources once the wait is over. PD forwards requests to its leader on the server side, so any peer answers; the probe rotates through the peers so one dead PD does not eat every poll.
| // same short-lived client as limitStorage(); PDClient has no close() | ||
| PDClient pdClient = PDClient.create(pdConfig); | ||
| try { | ||
| Metapb.PDConfig pd = pdClient.getPDConfig(); |
There was a problem hiding this comment.
pd.stores_wait_timeout is not an upper bound for this startup wait. PDConfig.of(this.pdPeers) keeps the PD client's default 60-second gRPC deadline, so getPDConfig() here and each getActiveStores() call inside waitForStores() can block for up to 60s; the supplier is invoked before the deadline check as well. With pd.stores_wait_timeout=20 and a black-holed PD, startup can stall well past 20s. Please propagate a per-call deadline bounded by the remaining wait budget (or otherwise time-bound the RPC) and add a delayed/unreachable-PD test.
There was a problem hiding this comment.
Done in 3d64daa. Every probe call gets withDeadlineAfter(min(remaining budget, 5 s poll)), and the budget is checked before the call, not after. testBlackholedPdStaysWithinTheBudget: a probe that hangs for its whole deadline on every call receives shrinking deadlines (all ≤ 1 s at a 1 s poll) and the whole wait ends below timeout + one poll. On the lab with pd.peers=192.168.80.250:8686 (a black hole) and pd.stores_wait_timeout=20: Waiting for the PD cluster: 192.168.80.250:8686: UNAVAILABLE (16s left … 10s … 4s), then Timed out after 20s waiting for the PD cluster to be ready (192.168.80.250:8686: UNAVAILABLE); start the stores first or raise pd.stores_wait_timeout, exit 1 after 31 s (10 s of JVM boot plus the 20 s budget). Log: results/issue-3203/fix/after-v2-unreachable-pd-timeout20.log in https://github.com/SebastianGruza/hugegraph-validation.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the wait requires shard_count active stores on every start with usePD=true, not only on a cold start, so restarting the server while one of three stores is down now blocks for pd.stores_wait_timeout and exits, although PD reports the cluster as usable. Evidence: static read of GraphManager.waitForActiveStores at 0084e77, PD ConfigService (served PDConfig has no min_store_count), StoreNodeService.allocShards/checkStoreStatus. Latest-head CI is green.
| PDClient pdClient = PDClient.create(pdConfig); | ||
| try { | ||
| Metapb.PDConfig pd = pdClient.getPDConfig(); | ||
| int required = pd.getMinStoreCount() > 0 ? pd.getMinStoreCount() : |
There was a problem hiding this comment.
PDConfig PD serves, min_store_count is always 0 (ConfigService only sets partition and shard count), so required is shard_count on every cluster, and this runs on every start, not just the first boot.
That turns a normal restart into a failure: 3 stores, default-shard-count: 3, one store down for maintenance. Each raft group still has 2 of 3 replicas, PD's checkStoreStatus() reports Cluster_OK (it compares against pd.initial-store-count and a per-group majority), and before this PR the server opened its graphs. Now getActiveStores() returns 2, the loop waits 300 s and throws. The count is also not what PD enforces on first boot: allocShards() checks pd.initial-store-count and caps shards at min(shard_count, stores), so a cluster with fewer stores than shard_count never starts, and one with initial-store-count above shard_count still hits the original retry ceiling.
Requested change: gate the wait on PD's own readiness instead of a derived count, for example poll pdClient.getClusterStats() until the state is Cluster_OK (which already encodes initial-store-count and the majority check), or at least skip the wait once shard groups exist. Add a test for the restart-with-a-store-down case.
There was a problem hiding this comment.
Done in 3d64daa, thanks, that case was a real regression. The probe first asks queryPartitions with an empty query: if the cluster has any partition it is ready and nothing is waited for, whatever PD thinks of the stores at that moment. Only a cluster without partitions (first boot) waits, and it waits for getClusterStats() == Cluster_OK, which is exactly what allocShards() needs, with no count derived on the server side. One correction to your description: checkStoreStatus() also sets Cluster_Not_Ready when fewer than pd.initial-store-count stores are active (StoreNodeService.java:831-835), so with three stores and initial-store-count: 3 Cluster_OK on its own would block a restart with one store down as well; hence "partitions exist" as the first criterion and Cluster_OK only for an empty cluster.
Measured on the lab (1 PD + 3 Store + 1 Server from a tarball, usePD=true): a cold start with stores at +5 s / +71 s → 14 progress lines carrying PD's message (The number of active stores is 1, less than pd.initial-store-count:3), PD cluster ready after 70s: PD reports Cluster_OK, 0 × 105, exit 0 after 81 s, REST 200; a restart with the node2 store killed with kill -9 (port 8500 closed) → PD cluster ready after 0s: cluster already has 24 partition(s), exit 0 after 8 s, REST 200; PD still listed that store as Up 200 s later, which only confirms that the skip must not depend on PD's view of the stores. testInitialisedClusterIsNotWaitedFor covers the case (one probe, zero wait). Logs: results/issue-3203/fix/after-v2-*.log.
…n an initialised cluster Review round 1 of apache#3210: - a closable one-shot gRPC probe (one plaintext channel per PD peer, no watchers, closed after the wait) instead of a leaked PDClient - every RPC gets a deadline bounded by the remaining wait budget, so pd.stores_wait_timeout is an upper bound even when PD is black-holed - a cluster that already has partitions is not waited for at all (a restart with one store down starts as before); only a cluster without partitions waits, and it waits for PD's own Cluster_OK, which is what allocShards() needs on the first boot - tests: initialised cluster, cold start, black-holed PD within budget, timeout keeps PD's last message
|
Round 1 in 3d64daa: a one-shot gRPC probe (a channel per PD peer, no watchers, closed after the wait) replaces the temporary |
- reuse the configured PD inbound message limit - cover partition responses larger than 4 MiB over gRPC - add the cold-start before and after diagram
- remove the diagram from the branch tree - retain the description image via its immutable URL - keep the code fix and regression test unchanged
imbajin
left a comment
There was a problem hiding this comment.
+1, the current scope is appropriate for #3203. The bounded wait before opening any graph addresses the cold-start failure while preserving existing-cluster startup and the generic Store retry behavior. The earlier review concerns and the probe response-size regression are addressed.
A non-blocking follow-up would be a lightweight PD bootstrap-status query, avoiding the full partition-list response, with probing encapsulated in a closable PD client helper. That broader API/client change does not need to expand this fix.
Validation: JDK 11 full reactor compilation and 5/5 focused tests passed, including a real gRPC response over 4 MiB that failed before the fix. CI for this head is still running; merge after it passes.
Purpose of the PR
With
usePD=truethe first hstore graph opened at startup (a localconf/graphsgraph, or the system graph created on the first boot) needspd.initial-store-countactive stores, and the store client gives up after a fixed 10 retries (about 38 s). Stores that register later than that make the server exit 1 on a cold start; under Kubernetes the container is restarted and usually succeeds the second time, without an orchestrator the server stays down.Before → after
Cluster_OK, bounded bypd.stores_wait_timeout.Main Changes
pd.stores_wait_timeoutin seconds: default300;0disables the gate and preserves the previous behavior. Applies whenusePD=true.GraphManagerbefore local graphs and PD metadata are loaded. An existing partition lets startup continue; an empty cluster waits for PD's ownCluster_OK, without deriving an active-Store threshold from replica count.min(remaining budget, 5 s); channels close after the wait and no watchers are created.NodeTxExecutorretry behavior unchanged. This is a bounded bootstrap wait, not a guarantee that all later graph operations succeed.Verifying these changes
Follow-up validation on JDK 11:
GraphManagerStoresWaitTest5/5 passed, including a real gRPC partition response over 4 MiB. The new regression test fails withRESOURCE_EXHAUSTEDbefore the inbound-limit fix and passes afterward.mvn editorconfig:format,git diff --check, andmvn clean compile -Dmaven.javadoc.skip=truepassed (all 38 reactor modules).Original
unit/core/GraphManagerStoresWaitTestcoverage (added toUnitTestSuite), 4/4 on JDK 11: an initialised cluster is not waited for (one probe, zero wait); a cold start waits until PD reports OK, with one unreachable answer retried; a black-holed probe gets deadlines that shrink with the budget and the total stays under timeout + one poll; the timeout message keeps PD's last message and names the option.Author-reported reproduction on 3 VMs (1 PD + 3 Store + 1 Server from a tarball,
usePD=true; same setup as in my comment on [Bug][HStore] Server exits 1 on cold start when Stores miss the 38s partition-lookup retry ceiling #3203, scriptcluster/repro_coldstart.sh, logs inresults/issue-3203/fix/of https://github.com/SebastianGruza/hugegraph-validation):1a15e762, cold start, stores +5 s / +71 s after PD, first booterror code = 105, backoff 1,1,1,2,3,4,5,6,7,8,upper limit : 10, exit 1 after 38 sWaiting for the PD cluster: Cluster_Not_Ready: The number of active stores is 1, less than pd.initial-store-count:3, thenPD cluster ready after 70s: PD reports Cluster_OK, 0 × 105, exit 0 after 81 s, REST 200kill -9, port closed), cluster initialisedPD cluster ready after 0s: cluster already has 24 partition(s), exit 0 after 8 s, REST 200 (PD still listed the store as Up 200 s later, so the skip must not depend on PD's store view)pd.peers=192.168.80.250:8686(black hole),pd.stores_wait_timeout=20Waiting for the PD cluster: 192.168.80.250:8686: UNAVAILABLE (16s left … 10s … 4s),Timed out after 20s waiting for the PD cluster to be ready (192.168.80.250:8686: UNAVAILABLE); start the stores first or raise pd.stores_wait_timeout, exit 1 after 31 s (10 s JVM boot + 20 s budget)Notes
GraphManagerconstructor, not increateSysGraphIfNeed(): with a localconf/graphshstore graph that local graph is opened beforeloadMetaFromPD()and already trips the retry ceiling (a first version that waited only before the system graph did nothing on the VMs).Cluster_OKalone would not do for restarts:StoreNodeService.checkStoreStatus()reportsCluster_Not_Readywhenever fewer thanpd.initial-store-countstores are active, so with three stores andinitial-store-count: 3a restart with one store down would wait too. Hence "partitions exist" as the first criterion andCluster_OKonly for an empty cluster.[wait-storage]gate in the image entrypoint from feat(helm): add HStore deployment chart #3132 stays the right thing for containers; this change covers the tarball and any start without an orchestrator.The terminal-log forwarding suggestion in #3203 is outside this change; container logging and orchestration gates remain separate concerns.