Conversation
He-Pin
reviewed
Aug 4, 2026
pjfanning
added a commit
to pjfanning/incubator-pekko-management
that referenced
this pull request
Aug 25, 2026
Motivation: Follow-up on the review of this branch. The 30 second wait added here and the 10 second timer in `ClusterBootstrap.ensureSelfContactPoint` are coupled - the wait is only correct while it outlasts the timer that completes the promise - but nothing expressed that. Raising the timer would silently make the decider time out first, replacing the deliberate "'Bootstrap.selfContactPoint' was NOT set" error with a bare TimeoutException from elsewhere. The `lazy val` re-runs its initialiser after a throw, so in the case this change exists for - `start()` never ran, so nothing ever completes the promise - every `canJoinSelf` call blocks a dispatcher thread for the full timeout again. The previous `Duration.Inf` parked one thread once; this parks one per probe, for as long as the contact point stays unset. There was no test. `SelfAwareJoinDeciderSpec` only covers the path where the contact point has already been set, and a 30 second hardcoded timeout cannot be exercised in a test anyway. Modification: Move the timer's duration to `ClusterBootstrap.SelfContactPointTimeout` and derive the decider's wait from it, behind a `protected def` a test can override. Replace the `lazy val` with an `AtomicReference` that caches a resolved value. A failure is deliberately not cached, so a contact point set later is still picked up, but the blocking wait is paid at most once: reaching the timeout means the promise has nothing to complete it, so later callers check the promise without blocking and fail fast until it does complete. Resolve against the promise directly rather than mapping it first, so that the already-completed case needs no dispatcher hop and `value` is meaningful. Result: The two timeouts cannot drift apart. An unset contact point costs one blocking wait rather than one per probe, and is still picked up if it arrives late. Tests: - sbt "management-cluster-bootstrap/test" - 54 succeeded, 0 failed (48 before) - New SelfContactPointResolutionSpec covers resolution, caching, the timeout, the block-once behaviour, late setting after a timeout, and the ordering between the two timeouts - Directional: with the block-once guard removed so that every call waits, "block for the timeout only once, then fail fast" FAILS with "505796658 nanoseconds was not less than 250 milliseconds" - sbt "management-cluster-bootstrap/mimaReportBinaryIssues" - success - sbt "management-cluster-bootstrap/scalafmtCheck" "management-cluster-bootstrap/Test/scalafmtCheck", "+headerCheckAll" - clean References: Refs apache#908
pjfanning
added a commit
to pjfanning/incubator-pekko-management
that referenced
this pull request
Aug 25, 2026
Motivation: ClusterBootstrap.SelfContactPointTimeout is private[bootstrap], so scaladoc cannot resolve a [[...]] link to it and unidoc fails the Docs compile job with 'Could not find any member to link'. Modification: Refer to it as code rather than as a link, in both places. Result: Scaladoc generates again. Tests: - sbt "unidoc; docs/paradox" - success, which is the exact command the Docs compile job runs References: Refs apache#908
Motivation: Follow-up on the review of this branch. The 30 second wait added here and the 10 second timer in `ClusterBootstrap.ensureSelfContactPoint` are coupled - the wait is only correct while it outlasts the timer that completes the promise - but nothing expressed that. Raising the timer would silently make the decider time out first, replacing the deliberate "'Bootstrap.selfContactPoint' was NOT set" error with a bare TimeoutException from elsewhere. The `lazy val` re-runs its initialiser after a throw, so in the case this change exists for - `start()` never ran, so nothing ever completes the promise - every `canJoinSelf` call blocks a dispatcher thread for the full timeout again. The previous `Duration.Inf` parked one thread once; this parks one per probe, for as long as the contact point stays unset. There was no test. `SelfAwareJoinDeciderSpec` only covers the path where the contact point has already been set, and a 30 second hardcoded timeout cannot be exercised in a test anyway. Modification: Move the timer's duration to `ClusterBootstrap.SelfContactPointTimeout` and derive the decider's wait from it, behind a `protected def` a test can override. Replace the `lazy val` with an `AtomicReference` that caches a resolved value. A failure is deliberately not cached, so a contact point set later is still picked up, but the blocking wait is paid at most once: reaching the timeout means the promise has nothing to complete it, so later callers check the promise without blocking and fail fast until it does complete. Resolve against the promise directly rather than mapping it first, so that the already-completed case needs no dispatcher hop and `value` is meaningful. Result: The two timeouts cannot drift apart. An unset contact point costs one blocking wait rather than one per probe, and is still picked up if it arrives late. Tests: - sbt "management-cluster-bootstrap/test" - 54 succeeded, 0 failed (48 before) - New SelfContactPointResolutionSpec covers resolution, caching, the timeout, the block-once behaviour, late setting after a timeout, and the ordering between the two timeouts - Directional: with the block-once guard removed so that every call waits, "block for the timeout only once, then fail fast" FAILS with "505796658 nanoseconds was not less than 250 milliseconds" - sbt "management-cluster-bootstrap/mimaReportBinaryIssues" - success - sbt "management-cluster-bootstrap/scalafmtCheck" "management-cluster-bootstrap/Test/scalafmtCheck", "+headerCheckAll" - clean References: Refs apache#908
Motivation: ClusterBootstrap.SelfContactPointTimeout is private[bootstrap], so scaladoc cannot resolve a [[...]] link to it and unidoc fails the Docs compile job with 'Could not find any member to link'. Modification: Refer to it as code rather than as a link, in both places. Result: Scaladoc generates again. Tests: - sbt "unidoc; docs/paradox" - success, which is the exact command the Docs compile job runs References: Refs apache#908
Motivation: `SelfAwareJoinDecider.selfContactPoint` resolves eagerly, so `LowestAddressJoinDecider.decide` can throw before it ever constructs a Future. `BootstrapCoordinator.decide()` only guards the returned Future with `recover`, which never sees a synchronous throw. The exception escapes with `decisionInProgress` already set to true, and that flag is only reset on receipt of a JoinDecision, so every later DecideTick short-circuits on "Previous decision still in progress" and the coordinator stops deciding for the rest of its life. This is reachable once the decider's wait no longer outlasts the promise's own timeout, which is exactly what the fail-fast path introduced here can do. Modification: - Wrap `joinDecider.decide(info)` so a NonFatal synchronous throw becomes a failed Future, letting the existing `recover` log it and fall back to KeepProbing, which resets the flag. - Explain why the two fields in SelfAwareJoinDecider are atomic, given that in-tree callers are already serialised by the coordinator, and what each one holds. - Assert the caching test's actual property rather than tuple reference equality, which pinned an implementation detail. Result: An unset self contact point can no longer stop cluster bootstrap permanently; the coordinator keeps probing and recovers if the contact point is set later. Tests: - New BootstrapCoordinatorSpec case "keep making decisions instead of wedging". Directional: with the try/catch removed it fails with "1 was not greater than or equal to 2" - a single decide() call, then silence. - sbt "management-cluster-bootstrap/test" - 55 succeeded, 0 failed. - sbt "management-cluster-bootstrap/mimaReportBinaryIssues" - success. - scalafmt --mode diff-ref=origin/main and sbt +headerCheckAll - clean. References: Refs apache#908
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
SelfAwareJoinDecider.selfContactPointawaitedClusterBootstrap(system).selfContactPointwithDuration.Inf. The comment justified that with "the future has a timeout", which is only true onceClusterBootstrap.start()has run:ensureSelfContactPoint()schedules the timer that fails the promise, and it is called fromstart()alone. Bind the bootstrap routes manually and the promise is never completed by anything, so the await never returns.Two further problems came out of review:
TimeoutExceptionfrom somewhere else.lazy valdoes not fix the repeated-blocking case, because alazy valre-runs its initialiser after a throw. In exactly the scenario this PR exists for, everycanJoinSelfcall would block a dispatcher thread for the full timeout again —Duration.Infparked one thread once, this parks one per probe.Modification
ClusterBootstrap.SelfContactPointTimeoutand derive the decider's wait from it, behind aprotected def selfContactPointTimeoutthat a test can override. The effective value is unchanged.AtomicReference. A failure is deliberately not cached, so a contact point set later is still picked up, but the blocking wait is paid at most once: reaching the timeout means nothing will complete the promise, so subsequent callers check it without blocking and fail fast until it does complete.Future#valueis meaningful.canJoinSelfreuses theselfit already resolved instead of asking again.Result
The two timeouts cannot drift apart. An unset contact point costs one blocking wait rather than one per probe, and is still picked up if it arrives late.
Tests
sbt "management-cluster-bootstrap/test"— 54 succeeded, 0 failed (48 before this PR).SelfContactPointResolutionSpeccovers resolution, caching, the timeout, the block-once behaviour, setting a contact point after a timeout, and the ordering between the two timeouts.505796658 nanoseconds was not less than 250 milliseconds.sbt "management-cluster-bootstrap/mimaReportBinaryIssues"— success.sbt "management-cluster-bootstrap/scalafmtCheck" "management-cluster-bootstrap/Test/scalafmtCheck"and+headerCheckAll— clean.References
None - found by review of
SelfAwareJoinDecider