You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/05/07 02:57:14 UTC
[GitHub] [incubator-ratis] juaby opened a new pull request #93: 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 opened a new pull request #93:
URL: https://github.com/apache/incubator-ratis/pull/93
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] juaby commented on pull request #93: RATIS-931. Fixed Add a raft group to all peers currently registered
Posted by GitBox <gi...@apache.org>.
juaby commented on pull request #93:
URL: https://github.com/apache/incubator-ratis/pull/93#issuecomment-641847686
> 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's okay to verify by documentation (because the members of available workers == the members of new raft groups), but when the working node starts more than 3, when a log is created, the metadata service broadcasts adding the raft group to an unnecessary out-of-raft-group node.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] juaby edited a comment on pull request #93: RATIS-931. Fixed Add a raft group to all peers currently registered
Posted by GitBox <gi...@apache.org>.
juaby edited a comment on pull request #93:
URL: https://github.com/apache/incubator-ratis/pull/93#issuecomment-641847686
> 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #93: 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
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #93:
URL: https://github.com/apache/incubator-ratis/pull/93#issuecomment-625036341
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] juaby edited a comment on pull request #93: RATIS-931. Fixed Add a raft group to all peers currently registered
Posted by GitBox <gi...@apache.org>.
juaby edited a comment on pull request #93:
URL: https://github.com/apache/incubator-ratis/pull/93#issuecomment-641847686
> 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. Regarding 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 three 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 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org