imbajin commented on code in PR #3164:
URL: https://github.com/apache/hugegraph/pull/3164#discussion_r4028447564
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/business/BusinessHandler.java:
##########
@@ -230,6 +230,14 @@ void lock(String path) throws InterruptedException,
void unlock(String path);
+ /**
+ * Non-blocking attempt to reserve the compactRange() window for partition
{@code id}.
+ * Returns false if a compaction is actively running for that partition
right now.
+ */
+ boolean tryLockCompactionRange(int id);
Review Comment:
⚠️ Important. This PR adds three non-default abstract methods to the public
`BusinessHandler` interface (`tryLockCompactionRange`, `unlockCompactionRange`,
and `getPathLockState`). Any downstream plugin or test double that implements
the published `hg-store-core` interface will fail to compile when upgrading to
this head, even though the only in-repository implementation is updated.
Requested change: keep these compaction-lock helpers behind an internal
collaborator, or provide a compatibility-preserving API shape instead of
expanding the public interface with mandatory methods.
##########
hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/raft/PartitionStateMachine.java:
##########
@@ -198,7 +198,13 @@ public void onSnapshotSave(final SnapshotWriter writer,
final Closure done) {
done.run(Status.OK());
} catch (HgStoreException e) {
log.error(String.format("Raft %s onSnapshotSave failed. {}",
groupId), e);
- done.run(new Status(RaftError.EIO, e.toString()));
+ // A busy compaction-range lock is transient: jRaft's snapshot
scheduler
+ // retries independently, so report EBUSY rather than EIO to
avoid
+ // escalating to reportError()/restartRaftNode() (see
SnapshotExecutorImpl
+ // #onSnapshotSaveDone, which only escalates on EIO).
+ RaftError raftError = e.getCode() ==
HgStoreException.EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL ?
Review Comment:
⚠️ Important. The new `EC_RKDB_SNAPSHOT_SAVE_BUSY_FAIL` → `RaftError.EBUSY`
mapping is not tested at the state-machine boundary. The added tests assert the
exception from `SnapshotHandler`, but no test invokes
`PartitionStateMachine.onSnapshotSave()` and captures the `Closure` status; a
regression to mapping busy failures to `EIO` would still pass while causing
jRaft to escalate to `restartRaftNode()`. Please add direct coverage for busy →
`EBUSY`, ordinary save failure → `EIO`, and the final lock release.
--
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]
