bitflicker64 commented on code in PR #3362:
URL: https://github.com/apache/hugegraph/pull/3362#discussion_r4294521949
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/HeartbeatService.java:
##########
@@ -415,6 +442,29 @@ public void shutdown() {
}
}
+ private void reportOffline() {
+ if (fatalExitRequested.get() || storeInfo == null) {
+ return;
+ }
+ storeInfo.setState(Metapb.StoreState.Offline);
+ if (storeInfo.getId() == 0 || pdProvider == null ||
+ pdProvider.getPDClient() == null) {
+ return;
+ }
+ try {
+ // Preserve PD's current metadata, and never use Exiting for a
temporary restart.
+ var client = pdProvider.getPDClient();
+ client.getCache().removeStore(storeInfo.getId());
+ Metapb.Store current = client.getStore(storeInfo.getId());
+ if (current.getState() == Metapb.StoreState.Up) {
+
client.updateStore(current.toBuilder().setState(Metapb.StoreState.Offline).build());
Review Comment:
Important: A graceful restart of one Store now makes the other Stores reject
writes for the whole restart when `initial-store-count` equals the Store count.
The Helm chart always configures this (`HG_PD_INITIAL_STORE_COUNT` is
`store.replicas` in `pd-statefulset.yaml`), and so does
`docker-compose-3pd-3store-3server.yml`.
The chain at this head:
1. This `updateStore(Offline)` goes through `PDService.setStore` to
`StoreNodeService.updateStore`, whose Offline branch calls
`storeInfoMeta.removeActiveStore`.
2. The next `heartBeat` from any other Store calls `checkStoreStatus()`.
That method sees `activeStores.size()` (2) < `getMinStoreCount()` (3) and sets
`Cluster_Not_Ready`.
3. The other Stores cache that state in `HeartbeatService.storeHeartbeat`.
`isClusterReady()` then returns false, and `HgStoreNodeService.addRaftTask`
fails every write with `CLUSTER_NOT_READY`.
4. When the Store comes back, `register(Offline)` (`StoreNodeService` lines
215-219) removes it from the active set again and calls `checkStoreStatus()`
while it restores. The cluster becomes ready again only after the second, Up
registration.
Before this PR, nothing contacted PD on shutdown, and the keepalive entry
(`store.keepAlive-timeout`, default 300 s) kept a Store that restarted within
that time in the active set, so the cluster stayed `Cluster_OK`. Every shard
group still has a 2/3 majority in this case, so the outage comes from the store
count alone.
Requested change: do not let a temporary restart (shutdown report or restore
registration) drop the Store from the set that `checkStoreStatus` counts
against `initial-store-count`. One way is to record the Offline state for
visibility without calling `removeActiveStore`. Another is to have
`checkStoreStatus` not count a Store that reported a graceful Offline as
missing. Please add a PD test with 3 Stores and min count 3: one Store reports
Offline, and a `heartBeat` from another Store still returns `Cluster_OK`.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/HeartbeatService.java:
##########
@@ -200,10 +200,36 @@ protected void registerStore() {
this.storeInfo.setStoreAddress(IpUtil.getNearestAddress(options.getGrpcAddress()));
this.storeInfo.setRaftAddress(IpUtil.getNearestAddress(options.getRaftAddress()));
+ // Obtain the identity needed by partition recovery without
advertising this Store Up.
+ this.storeInfo.setState(Metapb.StoreState.Offline);
Review Comment:
Important: This Offline registration also runs when a Store that is already
serving reconnects, not only at startup. When the heartbeat stream fails (the
callback at line 243), or `heartbeatFailCount` passes
`MAX_HEARTBEAT_RETRY_COUNT`, `onStateChanged(Offline)` sends the loop back into
`registerStore()`. This line then sends Offline.
In PD, `lastStore` is Up, so `register` takes the new `restoring` branch. It
calls `removeActiveStore`, broadcasts `NODE_EVENT_TYPE_NODE_OFFLINE` through
`PDService`'s status listener, and calls `checkStoreStatus()`. Meanwhile, this
Store re-runs `loadPartition`/`syncPartitionsFromPD` before it registers Up
again. The partition engines kept running the whole time, so nothing needs
restoring.
Before this PR the same reconnect registered Up directly and PD saw no state
change. A PD leader change closes every Store's stream, so after it all Stores
flap Offline together. With `initial-store-count` equal to the Store count, any
Store that heartbeats during that window caches `Cluster_Not_Ready` and rejects
writes.
Requested change: send the Offline-then-restore handshake only until this
process has completed a restore once, for example with a `restored` flag set
after the first successful `stateChanged`. Later re-registrations can then
request Up directly as before. Please add a `StoreRegistrationTest` case: after
a successful registration and `onStateChanged(Offline)`, the next
`registerStore()` sends only Up.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/HgStoreEngine.java:
##########
@@ -296,13 +291,13 @@ public void restoreLocalPartitionEngine() {
try {
createPartitionEngine(p);
} catch (Exception e) {
- log.error("Partition {}-{} restore exception {}",
p.getGraphName(),
- p.getId(), e);
+ throw new IllegalStateException("Failed to restore
partition " +
Review Comment:
Important: Throwing inside this `forEach` stops the loop at the first
partition that fails, so partitions later in the iteration are never restored.
Each retry (every second, from `registerStore`) iterates in the same order,
hits the same failure first, and stops at the same place.
Before this PR, the failure was logged and every other partition still got
its `PartitionEngine`. Now, with one unrecoverable group (for example
`raftGroupService.start` returning null, which makes
`raftNode.addReplicatorStateListener` throw), the Store stays Offline. The raft
groups whose replicas come after that group in the iteration then run one
replica short for as long as the failure lasts. Each retry also re-runs
`syncPartitionsFromPD` and re-registers with PD once a second.
Requested change: try every partition, collect the failures, and throw one
exception after the loop. That keeps the Store Offline as intended while the
healthy local replicas still start. Please also add a test where the first of
two partitions fails and the second still gets an engine.
--
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]
