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

Reduce failures to connect from fatal errors to warnings #169

Merged
merged 1 commit into from
Sep 8, 2022

Conversation

metalicjames
Copy link
Member

Follow up from #168. This PR reduces all failures to connect from fatal errors to warnings. This should eliminate the startup order requirement of the system components.

I think I got them all, but it's possible I missed one.

@wadagso-gertjaap
Copy link
Collaborator

Without this the 2PC system cannot start up regardless of the order:

The 2PC sentinel creates separate network clients for each of the other sentinels (for the attestation feature) - meaning if connecting to it fails, the sentinel fails to start up.

Combined with the fact that the sentinel doesn't start listening before it has connected to other sentinels means the system can't ever start up.

Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

utACK. There are a couple of now-misleading comments that need to be updated.

tests/unit/archiver_test.cpp Outdated Show resolved Hide resolved
tests/unit/archiver_test.cpp Outdated Show resolved Hide resolved
@HalosGhost HalosGhost linked an issue Sep 8, 2022 that may be closed by this pull request
3 tasks
Copy link
Collaborator

@HalosGhost HalosGhost left a comment

Choose a reason for hiding this comment

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

Thank you for fixing those comments. This looks like a really simple change that both closes out the issue introduced by #135 and has a nice improvement in eliminating the not-particularly-well-documented component startup-order requirement.

Looks great to me.

@kylecrawshaw
Copy link
Contributor

Thanks for changing this @metalicjames . This fixes the issues I observed in #167

@HalosGhost HalosGhost merged commit b9a2597 into mit-dci:trunk Sep 8, 2022
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.

Sentinels fail to start coordinator client
4 participants