imbajin commented on code in PR #3362:
URL: https://github.com/apache/hugegraph/pull/3362#discussion_r4346582144
##########
hugegraph-pd/hg-pd-client/src/main/java/org/apache/hugegraph/pd/client/PDClient.java:
##########
@@ -382,14 +382,34 @@ private String getLeaderIp(PDBlockingStub stub) {
* @return
*/
public long registerStore(Metapb.Store store) throws PDException {
+ return registerStoreResponse(store).getStoreId();
+ }
+
+ public Metapb.Store registerStoreWithState(Metapb.Store store) throws
PDException {
+ Pdpb.RegisterStoreResponse response = registerStoreResponse(store);
+ if (response.hasStore()) {
+ if (config.isEnableCache() && response.getStore().getId() != 0) {
+ cache.addStore(response.getStore().getId(),
response.getStore());
+ }
+ return response.getStore();
+ }
+ if (response.getStoreId() == 0) {
+ return store.toBuilder().setId(0).build();
+ }
+ // Older PDs return only the identity. Avoid a stale Offline entry
from an earlier handshake.
+ cache.removeStore(response.getStoreId());
+ return getStore(response.getStoreId());
Review Comment:
⚠️ Important: When an older PD returns only `store_id`, this fallback makes
a second `getStore()` call before returning the allocated ID. If registration
succeeds but that lookup transiently fails, `DefaultPdProvider.registerStore()`
never sets the Store's ID, so its next retry registers ID 0 and
`newStoreNode()` persists a different ID. This can leave orphan Store records;
for initial-listed addresses, abandoned registrations can also keep active
leases until expiry. Please retain the returned ID before this fallible lookup
and retry the lookup using that ID, with a regression for lookup failure after
registration. Evidence: exact-head `registerStoreWithState()` →
`DefaultPdProvider.registerStore()` → `newStoreNode()` control flow.
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/StoreNodeService.java:
##########
@@ -206,13 +198,27 @@ 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 (restoring && storeState == Metapb.StoreState.Offline) {
+ // A returned Store is alive even while its engines recover. Renew
on each retry.
+ storeInfoMeta.keepStoreAlive(store);
Review Comment:
⚠️ Important: If an administrator marks a Store Offline while restoration is
still failing, the Store continues retrying Offline registration. This branch
recreates the active lease with `keepStoreAlive()`, then `checkStoreStatus()`
counts that TTL entry toward `minStoreCount` and shard availability even though
the canonical Store state is Offline. Please persist or fence administrative
Offline intent so later restore retries cannot renew the lease until explicit
reactivation, and add a regression that registers Offline after
`updateStore(Offline)`. Evidence: exact-head
`register()`/`updateStore()`/`checkStoreStatus()` paths and the
`HeartbeatService` registration loop.
--
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]
