imbajin commented on code in PR #3417:
URL: https://github.com/apache/hugegraph/pull/3417#discussion_r4393519344
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/meta/PartitionManager.java:
##########
@@ -327,6 +328,31 @@ private void loadPartitions() {
}
}
+ private Metapb.ShardGroup recoveryShardGroup(int partId) {
+ Metapb.ShardGroup group;
+ try {
+ group = pdProvider.getShardGroupDirect(partId);
+ } catch (RuntimeException e) {
+ throw new InvalidShardException("Cannot read recovery shard group
" + partId +
+ " from PD", e);
+ }
+ if (group == null || group.getId() != partId || group.getShardsCount()
== 0) {
+ throw new InvalidShardException("Invalid recovery shard group " +
partId +
+ " from PD", null);
+ }
+ Set<Long> stores = new HashSet<>();
+ for (var shard : group.getShardsList()) {
+ if (shard.getStoreId() == 0 || !stores.add(shard.getStoreId()) ||
+ (shard.getRole() != Metapb.ShardRole.Leader &&
+ shard.getRole() != Metapb.ShardRole.Follower &&
+ shard.getRole() != Metapb.ShardRole.Learner)) {
Review Comment:
**[P1] Preserve learner roles on the empty-log recovery fallback**
Accepting `Learner` here also needs a role-aware startup path when no
durable Raft configuration is available. A persisted partition with an existing
but empty `raft/<group>/log` directory passes `resetPartitionPath()`. If
the old derived group omitted this Store and PD now lists it as a learner, this
PR refreshes that group and enables its recovery. However, [normal engine
creation](https://github.com/apache/hugegraph/blob/da2de4d17f4be20e121f1f52ba7ff69700d31c70/hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/HgStoreEngine.java#L414-L420)
supplies `getPartitionPeers(shardGroup)` rather than a role-aware
configuration; [that
method](https://github.com/apache/hugegraph/blob/da2de4d17f4be20e121f1f52ba7ff69700d31c70/hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/meta/PartitionManager.java#L818-L831)
emits every member as a voter address. With no log/snapshot configuration to
restore, [JRaft 1.3.14 uses initialConf](https://github.
com/sofastack/sofa-jraft/blob/v1.3.14/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java#L1036-L1045),
so the intended learner starts with voter membership.
The role-erasing helper predates this PR, but refreshing a stale group that
previously excluded the local Store newly enables this unsafe fallback. The
durable-configuration test covers the safe nonempty recovery case; the
learner-only test currently checks metadata without starting the engine.
Please provide a role-aware initial `Configuration`, or fail closed for this
unsupported bootstrap case, and add a regression that starts recovery with a PD
learner and no durable log/snapshot configuration. This is a conditional
role-fidelity/safety finding from source tracing, not a runtime reproduction or
a claim of observed data loss. It independently confirms the [source-PR
finding, 🔗 hugegraph/hugegraph
#344](https://github.com/hugegraph/hugegraph/pull/344#discussion_r4393474891).
--
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]
