andyhuangdev commented on code in PR #11086:
URL: https://github.com/apache/ozone/pull/11086#discussion_r3837456321
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java:
##########
@@ -634,7 +634,12 @@ public CompletableFuture<TermIndex>
notifyInstallSnapshotFromLeader(
return CompletableFuture.supplyAsync(
() -> {
try {
- return ozoneManager.installSnapshotFromLeader(leaderNodeId);
+ TermIndex termIndex =
ozoneManager.installSnapshotFromLeader(leaderNodeId);
+ if (termIndex == null) {
+ throw new CompletionException(
+ new IOException("Failed to install snapshot from OM leader "
+ leaderNodeId));
+ }
+ return termIndex;
Review Comment:
Thanks for pointing this out. I agree that treating every `null` `TermIndex`
as an exception is too broad, since `null` is also used for cases where
snapshot installation is unavailable or should not proceed.
I propose moving the failure handling to
`OzoneManager.installSnapshotFromLeader()`. A genuine `installCheckpoint()`
failure would be propagated as an `IOException`, while the existing
non-exceptional `null` cases would retain their current semantics.
`OzoneManagerStateMachine` already converts an `IOException` into an
exceptional future for Ratis.
I will also update the tests to verify that:
- a `null` result still completes normally;
- an `IOException` completes the future exceptionally; and
- a checkpoint installation failure is propagated as an `IOException`.
Does this approach align with what you had in mind?
--
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]
