You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "CRZbulabula (via GitHub)" <gi...@apache.org> on 2026/06/10 07:38:51 UTC
[PR] Catch per-startup failures during ConfigNode leader warm-up [iotdb]
CRZbulabula opened a new pull request, #17898:
URL: https://github.com/apache/iotdb/pull/17898
## Background
Follow-up to #17821 ("Improve ConfigNode leader warm-up before serving"), addressing a review comment from @caideyipi.
## Problem
In `ConfigRegionStateMachine.becomeLeader()`, the parallel leader-service startups are joined with a barrier:
```java
CompletableFuture.allOf(startups).join();
```
Each startup is wrapped by `startInParallelIfEpochCurrent()`, whose Javadoc claims the returned future *"always completes normally so `CompletableFuture#allOf` acts as a clean join barrier."* But the body ran `startup.run()` **without any exception guard**:
```java
return CompletableFuture.runAsync(
() -> {
if (isCurrentLeaderServicesEpoch(epoch)) {
startup.run(); // unguarded
}
},
leaderServicesStartupPool);
```
If any of the parallel startups — CQ scheduler, pipe meta-sync/heartbeat, subscription meta-sync, metrics, cluster-id check, etc. — throws a `RuntimeException`, the corresponding future completes **exceptionally**. `allOf(startups).join()` then rethrows it as a `CompletionException`, which propagates out of `becomeLeader()` **before** `ProcedureManager.startExecutor()` and `markLeaderServicesReadyIfEpochCurrent()` run.
As a result `leaderServicesReady` is never set to `true`, and the ConfigNode keeps returning `CONFIG_NODE_LEADER_WARMING_UP` to clients **indefinitely** — the leader silently never becomes serviceable. The documented "always completes normally" invariant was simply false.
## Fix
Make the invariant true by catching and logging each startup's failure inside `startInParallelIfEpochCurrent()` so it can never escape into the join barrier:
- Each startup now runs inside a `try/catch`; a failure is logged at `ERROR` and swallowed, so a single misbehaving service can no longer abort the whole transition.
- A failed service stays unstarted until the next leadership transition, but the node still finishes warming up and begins serving — and the failure is observable via the error log instead of a silent hang.
- To make that log actionable, each startup is paired with a human-readable name through a small `LeaderServiceStartup` holder, so the log identifies exactly which service failed.
- The `startInParallelIfEpochCurrent()` Javadoc is updated to describe the actual (now-correct) behavior.
## Behavior change
| | Before | After |
|---|---|---|
| One parallel startup throws | `becomeLeader()` aborts; node stuck at `CONFIG_NODE_LEADER_WARMING_UP` forever | Failure logged with the service name; warm-up completes; node serves |
| Other startups | Their outcome irrelevant — transition already aborted | Continue and complete normally |
## Testing
- `mvn spotless:apply -pl iotdb-core/confignode` — clean.
- `mvn compile -pl iotdb-core/confignode` — compiles successfully.
The change is a self-contained defensive guard around existing private startup logic; no public surface or existing tests are affected.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Catch per-startup failures during ConfigNode leader warm-up [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula merged PR #17898:
URL: https://github.com/apache/iotdb/pull/17898
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Catch per-startup failures during ConfigNode leader warm-up [iotdb]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #17898:
URL: https://github.com/apache/iotdb/pull/17898#issuecomment-4668475325
@Caideyipi Thanks, this is a fair concern and I want to be explicit about the trade-off I'm making.
You're right that this turns every parallel startup into best-effort and still marks `leaderServicesReady` true, so in the worst case `confirmLeader()` can return SUCCESS while one leader-only service (pipe/CQ/subscription) stays disabled until the next leadership transition. I considered the three options you listed (required-vs-optional gating, keep-warming on required failure, retry/health gating) but I think for this PR best-effort is actually the right line, for a few reasons:
1. **These startups almost never fail.** They are the same activation calls that already ran successfully on every previous leadership transition; a `RuntimeException` here is a genuinely exceptional event (a bug or hard environmental fault), not an expected transient condition. So we're optimizing the handling of a near-zero-probability path.
2. **There is no good in-place recovery handle.** If `startCQScheduler()` / `startPipeMetaSync()` / `startSubscriptionMetaSync()` throws, none of them expose an idempotent "try again" entry point that's safe to call mid-epoch — re-running them isn't a free retry, and a retry/health-gating loop would have to reason about partial state each one left behind. Building that recovery machinery is a much larger change than the bug this PR fixes, and it would add real complexity to the warm-up path for a case that essentially never fires.
3. **`leaderServicesReady` blocking forever is strictly worse than best-effort serving.** The pre-fix behavior is: one throw aborts `becomeLeader()` *before* `markLeaderServicesReadyIfEpochCurrent()`, so the node returns `CONFIG_NODE_LEADER_WARMING_UP` **indefinitely** — the entire ConfigNode is unserviceable, including all the leader functionality that started fine, with no log pointing at the cause. The keep-warming-on-required-failure option you mention reproduces exactly this hang for required services; it just makes it intentional. Isolating the blast radius so one failed service can't take down the whole leader — and surfacing it via an ERROR log naming the service — is already a large improvement over a silent total stall.
So the design intent of this PR is deliberately narrow: **make the documented "always completes normally" invariant true, and make a failure observable instead of silent**, rather than try to auto-heal a service that we have no clean way to restart anyway.
If we do want required-vs-optional distinction or retry/health gating later, I'd rather do it as a focused follow-up that also gives each service a real idempotent restart path (so retry is meaningful) and a health endpoint to gate on — otherwise we're gating readiness on a flag we can't recover. Happy to file that as a separate issue. Does scoping it that way sound reasonable to you?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Catch per-startup failures during ConfigNode leader warm-up [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #17898:
URL: https://github.com/apache/iotdb/pull/17898#issuecomment-4667841184
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17898) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17898&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_iotdb&pullRequest=17898&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_iotdb&pullRequest=17898&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17898&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_iotdb&pullRequest=17898&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=17898)
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Catch per-startup failures during ConfigNode leader warm-up [iotdb]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #17898:
URL: https://github.com/apache/iotdb/pull/17898#issuecomment-4668144540
## [Codecov](https://app.codecov.io/gh/apache/iotdb/pull/17898?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `30 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 40.74%. Comparing base ([`11a178a`](https://app.codecov.io/gh/apache/iotdb/commit/11a178a3672fa10b739a69221c1bb80afd9b8722?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`83ac794`](https://app.codecov.io/gh/apache/iotdb/commit/83ac7942be6653de701a1afa4791fd50d32107d3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/iotdb/pull/17898?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...nsensus/statemachine/ConfigRegionStateMachine.java](https://app.codecov.io/gh/apache/iotdb/pull/17898?src=pr&el=tree&filepath=iotdb-core%2Fconfignode%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fiotdb%2Fconfignode%2Fconsensus%2Fstatemachine%2FConfigRegionStateMachine.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aW90ZGItY29yZS9jb25maWdub2RlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9pb3RkYi9jb25maWdub2RlL2NvbnNlbnN1cy9zdGF0ZW1hY2hpbmUvQ29uZmlnUmVnaW9uU3RhdGVNYWNoaW5lLmphdmE=) | 0.00% | [30 Missing :warning: ](https://app.codecov.io/gh/apache/iotdb/pull/17898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17898 +/- ##
============================================
+ Coverage 40.62% 40.74% +0.11%
- Complexity 2621 2623 +2
============================================
Files 5244 5244
Lines 362633 362646 +13
Branches 46684 46684
============================================
+ Hits 147315 147754 +439
+ Misses 215318 214892 -426
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/iotdb/pull/17898?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org