bitflicker64 commented on code in PR #3362:
URL: https://github.com/apache/hugegraph/pull/3362#discussion_r4298095213
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/pd/DefaultPdProvider.java:
##########
@@ -133,7 +133,10 @@ public long registerStore(Store store) throws PDException {
store);
long storeId = 0;
- Metapb.Store protoObj = store.getProtoObj();
+ Metapb.Store protoObj = store.getProtoObj().toBuilder()
+ .setState(store.getState() ==
Metapb.StoreState.Up ?
Review Comment:
Minor: With this change, the activation check a few lines below (line 143)
warns on every normal restart, not only for Pending Stores.
The first registration now sends `Offline`, and `StoreNodeService.register`
deliberately records the Store as `Offline` until the second registration.
`pdClient.getStore(storeId)` therefore returns `Offline`, and the Store logs
`Store {} is not activated, state is Offline` on every start. The client cache
is enabled (`setEnableCache(true)`), so that `Offline` record is cached. Unless
a `NODE_OFFLINE` event invalidates it first, the second, Up registration reads
the same cached record and logs the warning again. While a restore keeps
failing, the Offline registration repeats every second, and so does the
warning. Before this PR, registration requested `Up` (`Store.getProtoObj`
hardcodes it), so the warning appeared only for Stores still waiting for
activation.
Operators who look for this line to spot unactivated Stores now see it on
healthy restarts too. Please skip the check when the Store requested `Offline`
(or check for `Pending` only), and read the state from the response instead of
a cached `getStore`.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/HgStoreEngine.java:
##########
@@ -181,6 +180,8 @@ public synchronized boolean init(final HgStoreEngineOptions
opts) {
dataManager.setMetaManager(partitionManager);
dataManager.setCmdClient(hgCmdClient);
}
+ // Registration can restore partition engines immediately, so
initialize their owners first.
+ heartbeatService.init(options);
Review Comment:
Minor: Dropping `addStateListener(this)` here leaves
`HeartbeatService.stateListeners` empty in production; `git grep
addStateListener` at this head finds no other production caller. Restore now
runs only through the direct `storeEngine.stateChanged` call on the heartbeat
thread in `registerStore`.
`HeartbeatShutdownTest.testInvalidRecoveredShardExitsAfterNodeDestroy` still
wires the engine with `service.addStateListener(engine)` (line 230) and starts
restore with `service.onStateChanged(Up)`, which is the path this PR removed.
So the fatal exit from an invalid recovered shard is tested only through the
old async callback. Its new location, `requestExit(0)` on the heartbeat thread
while the shutdown hook joins that same thread, has no test. That path does
finish today: `stateChanged` returns, `registerStore` sees `terminated` and
returns, and the join completes. But a later change there would not be caught.
The comment in `HeartbeatService.shutdown` that producers "may have submitted
callbacks that create partition owners" is also stale now.
Please drive the recovery fixture through `registerStore()` (or `init`)
instead of the listener, so the test covers the production path, and update the
shutdown comment.
--
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]
