bitflicker64 commented on code in PR #3362:
URL: https://github.com/apache/hugegraph/pull/3362#discussion_r4297515851
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreNodeService.java:
##########
@@ -206,13 +198,25 @@ public Metapb.Store register(Metapb.Store store) throws
PDException {
}
}
+ // Updated Stores register Offline to obtain their identity before
restoring partitions.
+ // Older Stores still request Up, and Pending activation and duplicate
checks stay intact.
+ if (restoring && storeState == Metapb.StoreState.Up) {
+ storeState = Metapb.StoreState.Offline;
+ store = store.toBuilder().setState(storeState).build();
+ }
+
// Store information
storeInfoMeta.updateStore(store);
if (storeState == Metapb.StoreState.Up) {
// Update the store active status
storeInfoMeta.keepStoreAlive(store);
onStoreStatusChanged(store, Metapb.StoreState.Offline,
Metapb.StoreState.Up);
checkStoreStatus();
+ } else if (storeState == Metapb.StoreState.Offline &&
+ lastStore.getState() == Metapb.StoreState.Up) {
+ // Keep the existing keepalive window during a temporary restart;
do not renew it.
Review Comment:
Important: A Store that is back but still restoring never renews its
keepalive entry, so a restore that keeps failing now trips the
initial-store-count gate for the whole cluster.
This branch skips `keepStoreAlive`, and the Store sends no heartbeat until
restore succeeds. If one partition's `createPartitionEngine` keeps throwing,
the Store retries `register(Offline)` every second without renewing. Once the
pre-restart entry expires (`store.keepAlive-timeout`, 300 s),
`checkStoreStatus` counts 2 active Stores against `initial-store-count` 3 (the
Helm default), and `addRaftTask` on the other Stores rejects every write.
Before this PR the Store registered Up and kept heartbeating during restore, so
one broken partition left the cluster `Cluster_OK`. A restore that outlasts the
remaining entry has the same effect until it finishes.
`assertTemporaryOfflinePreservesReadiness(true)` asserts this outcome for a
Store that has already re-registered.
Please renew the entry while a returned Store restores, or document why one
bad partition should stop all writes.
##########
docker/README.md:
##########
@@ -233,6 +233,30 @@ Open `http://localhost:8088` and sign in as `admin` with
the password from `.env
</details>
+### Store registration during restart
+
+Stores first register `Offline` to obtain their persisted identity, restore
local
+partition engines, and then register `Up`. A restore failure keeps the Store
+offline and the heartbeat loop retries it. Graceful shutdown stops heartbeat
+producers before reporting `Offline` to PD; if PD is unavailable, normal
heartbeat
+expiry remains the fallback. A temporary stop does not request permanent
retirement.
+Offline reports retain the existing keepalive window without extending it, so a
+quick restart does not fail the cluster's initial-store-count gate. A Store
that
+does not return is still removed by normal expiry. Stream reconnects after a
+successful restore register `Up` directly without restoring the engines again.
+Restoration attempts every local partition before reporting any failures.
+
+Upgrade PD before Store to enable this registration handshake. Older Stores
keep
Review Comment:
Minor: This note says older PD versions ignore the restore gate, but it does
not say that the new shutdown report is harmful against an older PD. Upgrading
PD first avoids it on the way up, but not on a PD rollback or a Store-first
upgrade.
On a PD without this PR, `updateStore(Offline)` still calls
`removeActiveStore`. A new Store that stops gracefully against such a PD
removes itself from the active set. With `initial-store-count` equal to the
Store count, the other Stores then reject writes until it registers again. This
is the outage from the earlier review, now limited to mixed versions.
Please state this here and in `hugegraph-pd/docs/api-reference.md`, or send
the shutdown report only when PD is known to support it.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/HeartbeatService.java:
##########
@@ -200,10 +201,44 @@ protected void registerStore() {
this.storeInfo.setStoreAddress(IpUtil.getNearestAddress(options.getGrpcAddress()));
this.storeInfo.setRaftAddress(IpUtil.getNearestAddress(options.getRaftAddress()));
- long storeId = pdProvider.registerStore(this.storeInfo);
+ // Recovery is needed once per process, not when an established
stream reconnects.
+ boolean restoring = !restored;
+ this.storeInfo.setState(restoring ? Metapb.StoreState.Offline :
Metapb.StoreState.Up);
+ long storeId;
+ try {
+ storeId = pdProvider.registerStore(this.storeInfo);
+ } finally {
+ // Retry registration after a failure instead of heartbeating
an unregistered Store.
+ this.storeInfo.setState(Metapb.StoreState.Offline);
+ }
if (storeId != 0) {
storeInfo.setId(storeId);
storeMetadata.save(storeInfo);
+ if (restoring && storeEngine != null) {
+ try {
+ storeEngine.stateChanged(storeInfo,
Metapb.StoreState.Offline, Metapb.StoreState.Up);
Review Comment:
Minor: Partition engines now start before `startHeartbeatStream`, so
partition heartbeats throw `NullPointerException` while restore runs.
`DefaultPdProvider.pdPulse` is assigned only in `startHeartbeatStream` (line
251 here). The partition heartbeat thread starts in `init`. Once a restored
group elects a leader on this Store, `partitionHeartbeat()` calls
`pdProvider.partitionHeartbeat(statsList)`, which dereferences the null
`pdPulse`. `doPartitionHeartbeat` logs the error every 5 s, and PD gets no
partition stats from this Store until restore completes, or never while restore
keeps failing. Before this PR restore ran after the stream had started.
Please skip partition heartbeats until the pulse stream is connected, so
restore does not produce this error log and the gap is explicit.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreNodeService.java:
##########
@@ -288,6 +292,8 @@ public synchronized Metapb.Store updateStore(Metapb.Store
store) throws PDExcept
}
storeInfoMeta.updateStore(store);
+ // Temporary Offline reports retain the existing keepalive entry
without renewing it.
+ // Removing it here makes a rolling restart fail the
initial-store-count gate.
Review Comment:
Minor: Removing the Offline branch below changes every Offline update, not
only the Store's own shutdown report. That includes an operator's `POST
/v1/store/{id}` with `storeState: Offline` and gRPC `setStore`.
Before this PR, marking a crashed Store Offline removed its active entry at
once, so `allocShards`, `reallocShards` and `checkStoreStatus` stopped counting
it. Now the entry stays until keepalive expiry (up to 300 s), and new shard
groups can still be placed on the dead Store in that window.
This fits the separate shutdown path requested in the open thread on
`HeartbeatService` line 469: keep the removal for administrative Offline
updates and skip it only for the Store's temporary report. If the change is
intended, please document it.
--
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]
