Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClusterControl.joined should cause joining; fix waiting issues #1122

Merged
merged 4 commits into from
Jun 29, 2023

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Jun 29, 2023

bug: We wrongly waited on the self.node to become up but should wait for the other node in joined(node:)

improvement: We should initiate joining when not already trying to and someone asks try await joined(node:) since the equivalent join() makes it seem like joined would do this as well

@ktoso ktoso requested a review from yim-lee June 29, 2023 08:20
/// - within: Duration to wait for.
///
/// - Returns `Cluster.Member` for the joined node.
@discardableResult
public func joined(endpoint: Cluster.Endpoint, within: Duration) async throws -> Cluster.Member? {
try await self.waitFor(self.node, .up, within: within)
Copy link
Member Author

Choose a reason for hiding this comment

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

actual bug here, we must wait for endpoint not self.node ;)

@@ -314,7 +347,7 @@ public struct ClusterControl {
throw Cluster.MembershipError(.notFound(node, in: membership))
}

if atLeastStatus <= foundMember.status {
Copy link
Member Author

Choose a reason for hiding this comment

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

another mini bug; we should error out if the expected status is lower than the current one.

if they are the same, don't error -- we succeeded; the test covers this now

@ktoso
Copy link
Member Author

ktoso commented Jun 29, 2023

The failure is ClusterSingletonPluginClusteredTests.test_singletonByClusterLeadership_withLeaderChange and I've actually been looking into it now, may solve. It's a bit tricky coming back to this after a while, but I'll figure it out -- it's an unexpected dead letter :)

Unrelated to the PR though.

#1123

@ktoso
Copy link
Member Author

ktoso commented Jun 29, 2023

Failed: #1124 needs investigation

@ktoso
Copy link
Member Author

ktoso commented Jun 29, 2023

This improves things but we definitely still have some issues to look into here, I'll schedule time to work on those consistently now.

@ktoso ktoso added this to the 1.0.0-beta.4 milestone Jun 29, 2023
@ktoso ktoso changed the title ClusterControl.joined should cause joining for simpler usage ClusterControl.joined should cause joining; fix waiting issues Jun 29, 2023
@ktoso ktoso merged commit eaeb487 into apple:main Jun 29, 2023
@ktoso ktoso deleted the wip-fixups-joined branch June 29, 2023 22:41
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