Conversation
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR makes datanode container initialization failure handling fail-fast and preserves delayed endpoint failures across heartbeat cycles.
Changes:
- Adds a serialized initialization state machine with retained failure causes.
- Routes local startup failures toward datanode shutdown while preserving SCM retry behavior.
- Adds concurrency, timeout, and integration coverage.
File summaries
| File | Description |
|---|---|
| hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java | Updated as part of this pull request. |
| hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestDatanodeStateMachine.java | Updated as part of this pull request. |
| hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/states/datanode/TestRunningDatanodeState.java | Updated as part of this pull request. |
| hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java | Updated as part of this pull request. |
| hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/VersionEndpointTask.java | Updated as part of this pull request. |
| hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/datanode/RunningDatanodeState.java | Updated as part of this pull request. |
| hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/EndpointStateMachine.java | Updated as part of this pull request. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Catch both Exception and Error during container service startup. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation consistently handles initialization failures and includes focused concurrency and integration coverage.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Balanced
devmadhuu
left a comment
There was a problem hiding this comment.
Thanks @smengcl for the patch. Largely LGTM. But overall I have few questions:
- Does RATIS currently cleanly handles exception errors separately for each raft group. I mean if two raft groups are corrupted or some issue like volume failure while loading them, should DN continue to initialize and load remaining raft groups/pipelines and for problematic ones, ozone gets the status code etc ? So that ozone DN can eventually takes care of closure of those pipelines and let other OPEN and valid pipelines functional ? We currently run with
raft.server.log.corruption.policy = EXCEPTION(not overridden), so a corrupt log throws during server.start(). Would a per‑group skip/report path be safer than failing the entire node? I mean that would not be the part of this PR, but just wondering if this is how RATIS should handle ?
| try { | ||
| initializeContainerServices(clusterId); | ||
| initializingStatus.set(InitializingStatus.INITIALIZED); | ||
| } catch (IOException | RuntimeException | Error ex) { |
There was a problem hiding this comment.
Is it not too generic to handle all errors ? What if any OOM. It will not let InterruptedException also to interrupt the thread.
There was a problem hiding this comment.
This catch only records the terminal failure and immediately rethrows the same throwable, including OutOfMemoryError. It does not attempt recovery. Recording FAILED ensures other startup callers do not remain waiting for successful initialization.
At the endpoint boundary, the failure triggers fatal shutdown. Executor tasks capture Errors in their futures, so simply letting an Error escape would not reliably terminate the DN. Actual heap exhaustion can still prevent logging or orderly shutdown.
InterruptedException is not caught by this union. Acquiring a synchronized monitor is noninterruptible, which is a separate limitation.
|
|
||
| hddsDispatcher.init(); | ||
| hddsDispatcher.setClusterId(clusterId); | ||
| writeChannel.start(); |
There was a problem hiding this comment.
Right now we know some known issue in RATIS, but what if in future some other regression in RATIS makes hang forever, should we decouple this by having some timeout to avoid an infinite hold of initializationLock ?
There was a problem hiding this comment.
Yup this patch can't handle an initializer that never returns. The heartbeat timeout limits how long the caller waits, but does not cancel the underlying startup operation.
A startup watchdog would need an overall deadline and a terminal shutdown policy, accounting for legitimate long container and Raft log loading. Timing out lock acquisition alone would leave the original initializer running, and retrying partially initialized services would be unsafe.
I suggest handling that separately from this PR's exception-propagation fix.
|
Thanks @devmadhuu for taking a look.
Ratis 3.2.1 does not provide a per-group skip/report startup contract. It starts the groups in parallel and waits for their combined completion before starting the RPC servers. If a group’s startup fails, the overall server startup fails, even if other groups initialized successfully. ref:
Correct for the default The PR preserves both policies: it does not override the configured corruption policy or treat warnings as fatal. It triggers shutdown when startup actually throws.
It could improve availability for corruption isolated to one group. However, Ratis would need to prevent the failed group from serving requests, clean up its partially initialized resources, and report the failure so Ozone/SCM can handle the affected pipeline. Shared volume or server failures would also need to be distinguished from isolated group failures. Agreed that this would be a useful separate enhancement. This PR handles the existing server-start failure by terminating the DN with the original cause logged, rather than leaving initialization stuck. |
What changes were proposed in this pull request?
Datanode startup can hang indefinitely when container initialization fails, for example because a corrupted Raft log cannot be read.
OzoneContainer.start()already propagates the startup exception, but the existing failure handling has several gaps:INITIALIZING, leaving subsequent callers waiting indefinitely forINITIALIZED.VersionEndpointTaskdoes not consistently treat local initialization failures as fatal: runtime exceptions can escape, while mostIOExceptions are handled as retryable communication failures.Proposed changes:
InitializingStatusand addFAILED, preserving the original exception so waiting and subsequent callers fail promptly without repeating initialization.What is the link to the Apache JIRA?
HDDS-16425
How was this patch tested?
TestOzoneContainer,TestRunningDatanodeState, andTestDatanodeStateMachine.Generated-by: Codex (GPT-6)