bitflicker64 commented on code in PR #532:
URL: https://github.com/apache/hugegraph-doc/pull/532#discussion_r4304096348


##########
content/en/docs/quickstart/hugegraph/hugegraph-helm-operations.md:
##########
@@ -31,10 +31,18 @@ Store wait and Helm test reuse existing component images 
with `curl` already ins
 
 PD listens on REST 8620, gRPC 8686 and Raft 8610; Store on REST 8520, gRPC 
8500 and Raft 8510; Server HTTP on 8080; Hubble HTTP on 8088.
 Client Services hide individual Pod addresses. NodePort is a separate 
allocated port on reachable nodes, not the application listener port.
-PD `/v1/ready` requires a Raft leader; `/v1/health` only confirms the 
listener. Store startup waits for a majority of ready PD peers.
+PD `/v1/ready` requires a Raft leader. On older images, `/v1/health` only 
confirms the listener.
+Store startup waits for a majority of ready PD peers.
 Server waits up to `server.storageWaitTimeoutSeconds` (default 300) before its 
application startup; adjust it for slow storage bootstrap.
 No local Helm rendering verifies live cluster reachability, volumes or JVM 
capacity.
 
+Images containing the [fatal PD health 
fix](https://github.com/apache/hugegraph/pull/3358) return `503` from 
`/v1/health`
+after a terminal Raft error or shutdown; ordinary elections and quorum loss 
remain live (`200`).

Review Comment:
   Minor: This sentence promises a `503` after every terminal Raft error, but 
apache/hugegraph#3358 at ae49a99e6 still has an open P2 thread from @imbajin 
showing a case where `/v1/health` stays `200`.
   
   In that thread (`RaftStateMachine.java:218`), JRaft 1.3.14 can queue 
`LEADER_STOP` and `START_FOLLOWING` behind the `ERROR` event. `onError` sets 
`STATE_ERROR`, then the queued `onStartFollowing` replaces it with 
`STATE_FOLLOWER`, and `StoreAPI.checkHealthy()` answers `200` on a node that 
can no longer recover. If #3358 merges as is, a reader who relies on this 
sentence, or on the `pd.livenessPath=/v1/health` override below, would expect a 
restart that never comes.
   
   Please hold this paragraph until #3358 keeps the error state across queued 
callbacks, then recheck the wording. The same applies to lines 37-38 of the 
Chinese page.



##########
content/en/docs/quickstart/hugegraph/hugegraph-helm-operations.md:
##########
@@ -127,6 +135,15 @@ Use `store.updateStrategy.type=OnDelete` for a controlled 
maintenance rollout, a
 Delete one Store Pod at a time. Before replacing the next, verify the previous 
Pod is Ready, has a fresh Store heartbeat and each shard group has its
 full membership and exactly one leader, read from the PD leader. PD's `Up` 
status alone does not prove completed shard recovery.
 PDBs limit voluntary eviction, not StatefulSet controller disruption. Stop on 
missing members or leaders.
+
+Images containing the [Store registration 
fix](https://github.com/apache/hugegraph/pull/3362) register `Up` after local 
engine restore
+and report `Offline` on graceful shutdown. Upgrade PD before Store to enable 
the restore gate.
+Temporary Offline reports preserve the existing keepalive window without 
renewing it; expiry still applies if the Store does not return.

Review Comment:
   Minor: "expiry still applies if the Store does not return" leaves out the 
case where the Store has returned but is still restoring. In that case it also 
does not renew its keepalive.
   
   At apache/hugegraph#3362 head 9c08bf6fb, `StoreNodeService.register` (lines 
210-220) calls `keepStoreAlive` only for `Up`. A Store that is restoring 
retries `register(Offline)` every second and sends no heartbeat until restore 
succeeds. If restore keeps failing, or downtime plus restore takes longer than 
`store.keepAlive-timeout` (300 s), the entry expires. `checkStoreStatus` then 
sets `Cluster_Not_Ready` when `initial-store-count` equals the Store count, 
which is the Helm default (`HG_PD_INITIAL_STORE_COUNT` = `store.replicas`). The 
other Stores then reject writes. This is the open Important thread on 
`StoreNodeService.java:217` in #3362.
   
   Please say that a restore taking longer than the keepalive window also 
expires the entry and blocks writes cluster-wide. Make the same change to line 
132 of the Chinese page, or wait for #3362 to fix it.



##########
content/en/docs/quickstart/hugegraph/hugegraph-helm-operations.md:
##########
@@ -127,6 +135,15 @@ Use `store.updateStrategy.type=OnDelete` for a controlled 
maintenance rollout, a
 Delete one Store Pod at a time. Before replacing the next, verify the previous 
Pod is Ready, has a fresh Store heartbeat and each shard group has its
 full membership and exactly one leader, read from the PD leader. PD's `Up` 
status alone does not prove completed shard recovery.
 PDBs limit voluntary eviction, not StatefulSet controller disruption. Stop on 
missing members or leaders.
+
+Images containing the [Store registration 
fix](https://github.com/apache/hugegraph/pull/3362) register `Up` after local 
engine restore
+and report `Offline` on graceful shutdown. Upgrade PD before Store to enable 
the restore gate.

Review Comment:
   Minor: "report `Offline` on graceful shutdown" is not guaranteed. PD refuses 
that report whenever the cluster is not already `Cluster_OK`.
   
   At apache/hugegraph#3362 head 9c08bf6fb, `HeartbeatService.reportOffline` 
sends `updateStore(Offline)`. That reaches `PDService.setStore` (lines 
469-480), which returns an error header for any `Offline` update while 
`clusterStats` is not `Cluster_OK`. The Store logs a warning and stops, and PD 
keeps it `Up` until keepalive expiry. So in a cluster that is already degraded, 
for example with another Store expired, a stopped Store still shows `Up`. That 
is the case where an operator most needs the signal. This is the open P2 thread 
on `HeartbeatService.java:469` in #3362.
   
   Please add that the report is skipped when the cluster is not `Cluster_OK`, 
or wait for #3362 to resolve it. Make the same change to line 130 of the 
Chinese page.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to