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

RATIS-931. Fixed Add a raft group to all peers currently registered #93

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

juaby
Copy link

@juaby juaby commented May 7, 2020

fixed In the log service, when creating a log, add a raft group to all group peers, where the group peers are not all peers currently registered

juaby and others added 2 commits May 7, 2020 10:21
pull from apache ratis
…l group peers, where the group peers are not all peers currently registered
@runzhiwang
Copy link
Contributor

You can open a jira at https://issues.apache.org/jira/projects/RATIS/issues, and add the jira number in the Pull Request title.
Besides, the Pull Request title is also too long.

@juaby juaby changed the title fixed In the log service, when creating a log, add a raft group to all group peers, where the group peers are not all peers currently registered RATIS-931: Fixed Add a raft group to all group peers, where the group peers are not all peers currently registered May 7, 2020
@juaby juaby changed the title RATIS-931: Fixed Add a raft group to all group peers, where the group peers are not all peers currently registered RATIS-931. Fixed Add a raft group to all group peers, where the group peers are not all peers currently registered May 7, 2020
@juaby juaby changed the title RATIS-931. Fixed Add a raft group to all group peers, where the group peers are not all peers currently registered RATIS-931. Fixed Add a raft group to all peers currently registered May 7, 2020
@bshashikant bshashikant requested a review from joshelser May 14, 2020 09:19
Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

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

Can we rename the member peers to be allKnownPeers or similar? Something to try to prevent such an error in the future.

What about a test? Did you try to write a unit test to catch this? I think having 4 available workers, create a log, and then look at the raft groups and see that it has 4 members instead of 3 would work.

@juaby
Copy link
Author

juaby commented Jun 10, 2020

Can we rename the member peers to be allKnownPeers or similar? Something to try to prevent such an error in the future.

What about a test? Did you try to write a unit test to catch this? I think having 4 available workers, create a log, and then look at the raft groups and see that it has 4 members instead of 3 would work.

Sorry, it took so long to reply to you. about the log service, it is okay to verify according to the document (because the number of worker nodes is equal to the number of new raft groups), but when more than 3 worker nodes are started, a log is created at the time, the metadata service will broadcast the addition of Raft groups to unnecessary nodes outside the new raft group.
Personal suggestion: Ratis server adds verification. when an additional raft group is received, verify that the new raft group members must contain the current working node

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.

3 participants