Skip to content

HDDS-11470. Report OM snapshot installation failures to Ratis - #11086

Open
andyhuangdev wants to merge 2 commits into
apache:masterfrom
andyhuangdev:HDDS-11470
Open

HDDS-11470. Report OM snapshot installation failures to Ratis#11086
andyhuangdev wants to merge 2 commits into
apache:masterfrom
andyhuangdev:HDDS-11470

Conversation

@andyhuangdev

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/HDDS-11470

When OM checkpoint installation fails, the snapshot-install callback previously
completed normally with a null result. Ratis could therefore treat the request
as completed without a successful snapshot installation.

Complete the callback exceptionally when OM reports a failed installation.
Add unit coverage for both successful and failed callback completion.

Test:

  • mvn -pl :ozone-manager -am test -Dtest=TestOzoneManagerStateMachine -DskipShade -DskipRecon -DskipDocs -Dsurefire.failIfNoSpecifiedTests=false
  • ./hadoop-ozone/dev-support/checks/checkstyle.sh

@chungen0126 chungen0126 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andyhuangdev for working on this.

throw new CompletionException(
new IOException("Failed to install snapshot from OM leader " + leaderNodeId));
}
return termIndex;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct approach. A null return value for termIndex shouldn't always result in an exception.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants