You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/07/09 13:48:25 UTC
[GitHub] [ratis] tisonkun opened a new pull request, #677: RATIS-1619. Validate server id in raft group if the group is not null
tisonkun opened a new pull request, #677:
URL: https://github.com/apache/ratis/pull/677
## What changes were proposed in this pull request?
Validate server id in raft group if the group is not null
## What is the link to the Apache JIRA
[RATIS-1619](https://issues.apache.org/jira/browse/RATIS-1619)
## How was this patch tested?
Add new tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on a diff in pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #677:
URL: https://github.com/apache/ratis/pull/677#discussion_r917850028
##########
ratis-common/src/main/java/org/apache/ratis/util/Preconditions.java:
##########
@@ -102,6 +102,11 @@ static <T> T assertNotNull(T object, String name) {
+ name + " = " + object + " == null, class = " + object.getClass());
}
+ static <T> T assertNotNull(T object, String format, Object... args) {
Review Comment:
Good to know. I made the decision since `Objects.requireNonNull` throws `NullPointerException` while these methods throws `IllegalStateException`. Perhaps Guava's `Precondition.checkArgument` fits better XD
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #677:
URL: https://github.com/apache/ratis/pull/677#issuecomment-1179636531
> The failure of TestPeerCommandIntegrationWithGrpc is related to this.
I'm pushing a commit to address style comments. Will investigate this case now.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #677:
URL: https://github.com/apache/ratis/pull/677
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #677:
URL: https://github.com/apache/ratis/pull/677#discussion_r917441527
##########
ratis-common/src/main/java/org/apache/ratis/util/Preconditions.java:
##########
@@ -102,6 +102,11 @@ static <T> T assertNotNull(T object, String name) {
+ name + " = " + object + " == null, class = " + object.getClass());
}
+ static <T> T assertNotNull(T object, String format, Object... args) {
Review Comment:
FYI, we usually use `Objects.requireNonNull(..)`. It is fine if we add this method.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #677: RATIS-1619. Validate server id in raft group if the group is not null
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #677:
URL: https://github.com/apache/ratis/pull/677#issuecomment-1179548124
cc @szetszwo @codings-dan
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #677:
URL: https://github.com/apache/ratis/pull/677#issuecomment-1179645220
```
Error: Failures:
Error: TestLeaderElectionWithSimulatedRpc>LeaderElectionTests.testEnforceLeader:235->LeaderElectionTests.enforceLeader:249 expected:<s[1]> but was:<s[3]>
Error: TestServerRestartWithSimulatedRpc>ServerRestartTests.testRestartWithCorruptedLogEntryWithException:345->BaseTest.testFailureCase:167->BaseTest.testFailureCase:159 The test "restart-fail-ChecksumException" does not throw anything.
Error: Errors:
Error: TestNettyDataStreamChainTopologyWithGrpcCluster>DataStreamAsyncClusterTests.testMultipleStreamsMultipleServersStepDownLeader:78->DataStreamAsyncClusterTests.runTestDataStreamStepDownLeader:82->DataStreamAsyncClusterTests.runTestDataStream:97 » TestTimedOut
Error: TestNettyDataStreamStarTopologyWithGrpcCluster.testMultipleStreamsMultipleServers » Completion
Error: TestNettyDataStreamStarTopologyWithGrpcCluster>DataStreamAsyncClusterTests.lambda$runTestDataStream$2:106 » Completion
```
* `TestLeaderElectionWithSimulatedRpc>LeaderElectionTests.testEnforceLeader` cannot reproduce locally.
* `TestServerRestartWithSimulatedRpc>ServerRestartTests.testRestartWithCorruptedLogEntryWithException` cannot reproduce locally, reported above.
* `TestNettyDataStreamChainTopologyWithGrpcCluster.testMultipleStreamsMultipleServersStepDownLeader` failed on master also, reported above.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #677:
URL: https://github.com/apache/ratis/pull/677#discussion_r917300504
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerImplUtils.java:
##########
@@ -47,14 +50,12 @@ public static RaftServerProxy newRaftServer(
RaftPeerId id, RaftGroup group, StateMachine.Registry stateMachineRegistry,
RaftProperties properties, Parameters parameters) throws IOException {
RaftServer.LOG.debug("newRaftServer: {}, {}", id, group);
- final RaftServerProxy proxy = newRaftServer(id, stateMachineRegistry, properties, parameters);
- proxy.initGroups(group);
- return proxy;
- }
- private static RaftServerProxy newRaftServer(
- RaftPeerId id, StateMachine.Registry stateMachineRegistry, RaftProperties properties, Parameters parameters)
- throws IOException {
+ if (group != null && !group.getPeers().isEmpty()) {
+ final Set<RaftPeerId> ids = group.getPeers().stream().map(RaftPeer::getId).collect(Collectors.toSet());
Review Comment:
Use RaftGroup.getPeer(id) instead of creating a set.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerImplUtils.java:
##########
@@ -47,14 +50,12 @@ public static RaftServerProxy newRaftServer(
RaftPeerId id, RaftGroup group, StateMachine.Registry stateMachineRegistry,
RaftProperties properties, Parameters parameters) throws IOException {
RaftServer.LOG.debug("newRaftServer: {}, {}", id, group);
- final RaftServerProxy proxy = newRaftServer(id, stateMachineRegistry, properties, parameters);
- proxy.initGroups(group);
- return proxy;
- }
- private static RaftServerProxy newRaftServer(
- RaftPeerId id, StateMachine.Registry stateMachineRegistry, RaftProperties properties, Parameters parameters)
- throws IOException {
Review Comment:
Please revert the unrelated changes. The original code is more clear.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #677: RATIS-1619. Validate server id in raft group if the group is not null
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #677:
URL: https://github.com/apache/ratis/pull/677#issuecomment-1179552749
It seems `org.apache.ratis.server.impl.MiniRaftCluster#putNewServers` depends on the group that doesn't contains the new peer. I have no idea yet whether it's a legacy logic we should update here or the original purpose of this ticket is invalid.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #677: RATIS-1619. Validate server id in raft group if the group is not null
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #677:
URL: https://github.com/apache/ratis/pull/677#issuecomment-1179559776
`org.apache.ratis.server.impl.RaftReconfigurationBaseTest#testReconfTwice` can be unstable with current version. Investigating...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #677:
URL: https://github.com/apache/ratis/pull/677#issuecomment-1179566275
Tests failed at:
```
Error: TestServerRestartWithSimulatedRpc>ServerRestartTests.testRestartWithCorruptedLogEntryWithException:345->BaseTest.testFailureCase:167->BaseTest.testFailureCase:159 The test "restart-fail-ChecksumException" does not throw anything.
Error: Errors:
Error: TestNettyDataStreamChainTopologyWithGrpcCluster.testMultipleStreamsMultipleServersStepDownLeader » Completion
```
* `TestServerRestartWithSimulatedRpc>ServerRestartTests.testRestartWithCorruptedLogEntryWithException` cannot be reproduced locally.
* `TestNettyDataStreamChainTopologyWithGrpcCluster.testMultipleStreamsMultipleServersStepDownLeader` failed on master also.
Thus I guess they are flaky tests. Please confirm.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] tisonkun commented on a diff in pull request #677: RATIS-1619. Validate server id in raft group if the group is not empty
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #677:
URL: https://github.com/apache/ratis/pull/677#discussion_r917327709
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerImplUtils.java:
##########
@@ -47,14 +50,12 @@ public static RaftServerProxy newRaftServer(
RaftPeerId id, RaftGroup group, StateMachine.Registry stateMachineRegistry,
RaftProperties properties, Parameters parameters) throws IOException {
RaftServer.LOG.debug("newRaftServer: {}, {}", id, group);
- final RaftServerProxy proxy = newRaftServer(id, stateMachineRegistry, properties, parameters);
- proxy.initGroups(group);
- return proxy;
- }
- private static RaftServerProxy newRaftServer(
- RaftPeerId id, StateMachine.Registry stateMachineRegistry, RaftProperties properties, Parameters parameters)
- throws IOException {
+ if (group != null && !group.getPeers().isEmpty()) {
+ final Set<RaftPeerId> ids = group.getPeers().stream().map(RaftPeer::getId).collect(Collectors.toSet());
Review Comment:
Thanks! I don't notice this method :P
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerImplUtils.java:
##########
@@ -47,14 +50,12 @@ public static RaftServerProxy newRaftServer(
RaftPeerId id, RaftGroup group, StateMachine.Registry stateMachineRegistry,
RaftProperties properties, Parameters parameters) throws IOException {
RaftServer.LOG.debug("newRaftServer: {}, {}", id, group);
- final RaftServerProxy proxy = newRaftServer(id, stateMachineRegistry, properties, parameters);
- proxy.initGroups(group);
- return proxy;
- }
- private static RaftServerProxy newRaftServer(
- RaftPeerId id, StateMachine.Registry stateMachineRegistry, RaftProperties properties, Parameters parameters)
- throws IOException {
Review Comment:
Reverted.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org