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

Handle gracefully if no node is master eligible #226

Merged
merged 3 commits into from
May 27, 2021

Conversation

tvainika
Copy link
Contributor

About this change: What it does, why it matters

Karapace configuration allows configuring node to not be eligible for master. Handle gracefully ie. read-only mode if all nodes are configured non-eligible for master.

Karapace configuration allows configuring node to not be eligible for
master.  Handle gracefully ie. read-only mode if all nodes are
configured non-eligible for master.
@tvainika tvainika requested a review from a team as a code owner May 24, 2021 13:48
Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

I think it doesn't make sense to have a node that is not master eligible set as master in the metadata.

karapace/master_coordinator.py Show resolved Hide resolved
karapace/master_coordinator.py Show resolved Hide resolved
karapace/master_coordinator.py Outdated Show resolved Hide resolved
@tvainika
Copy link
Contributor Author

I think it doesn't make sense to have a node that is not master eligible set as master in the metadata.

I changed are_we_master logic and renamed is_master_eligible to has_eligible_master, so I think this clarifies the logic

Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

I changed are_we_master logic and renamed is_master_eligible to has_eligible_master, so I think this clarifies the logic

My bad, I should have been clearer on what I meant by "have a node that is not master eligible set as master in the metadata.". This change is setting the variable master_url even if there is not master. That variable is of type Optional[str], so if its value is None then there should be no master, otherwise it should have the URL set to the current master.

The above means the variable has_eligible_master is unnecessary.

Copy link
Contributor

@hackaugusto hackaugusto left a comment

Choose a reason for hiding this comment

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

LGTM

@hackaugusto hackaugusto merged commit 6633aca into master May 27, 2021
@hackaugusto hackaugusto deleted the no-eligible-master branch May 27, 2021 09:27
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