You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/10/24 17:31:09 UTC
[GitHub] [iotdb] SzyWilliam opened a new pull request, #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
SzyWilliam opened a new pull request, #7712:
URL: https://github.com/apache/iotdb/pull/7712
It is for RatisConsensus.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1003944807
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -151,8 +159,16 @@ public void start() throws IOException {
@Override
public void stop() throws IOException {
- clientManager.close();
- server.close();
+ addExecutor.shutdown();
+ try {
+ addExecutor.awaitTermination(5, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ logger.warn("{}: interrupted when shutting down add Executor", this);
Review Comment:
I'll add the exception. Thanks!
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1003945691
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -463,6 +484,38 @@ public ConsensusGenericResponse changePeer(ConsensusGroupId groupId, List<Peer>
return ConsensusGenericResponse.newBuilder().setSuccess(reply.isSuccess()).build();
}
+ @Override
+ public ConsensusGenericResponse addNewNodeToExistedGroup(
+ ConsensusGroupId groupId, Peer newNode, List<Peer> originalGroup) {
+
+ CompletableFuture<ConsensusGenericResponse> addResp =
+ CompletableFuture.supplyAsync(() -> addPeer(groupId, newNode), addExecutor);
+
+ try {
+ Thread.sleep(500);
Review Comment:
done
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] yifuzhou commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
yifuzhou commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1003936952
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -151,8 +159,16 @@ public void start() throws IOException {
@Override
public void stop() throws IOException {
- clientManager.close();
- server.close();
+ addExecutor.shutdown();
+ try {
+ addExecutor.awaitTermination(5, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ logger.warn("{}: interrupted when shutting down add Executor", this);
Review Comment:
Should we add exception e to the log?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1003949655
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -127,6 +133,8 @@ public RatisConsensus(ConsensusConfig config, IStateMachine.Registry registry)
properties, Collections.singletonList(new File(config.getStorageDir())));
GrpcConfigKeys.Server.setPort(properties, config.getThisNodeEndPoint().getPort());
+ addExecutor = Executors.newSingleThreadExecutor(new IoTThreadFactory("ratis-add"));
Review Comment:
done.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1004056931
##########
consensus/src/main/java/org/apache/iotdb/consensus/IConsensus.java:
##########
@@ -106,6 +107,17 @@ public interface IConsensus {
*/
ConsensusGenericResponse changePeer(ConsensusGroupId groupId, List<Peer> newPeers);
+ /** Call this method on any member of originalGroup */
+ default ConsensusGenericResponse addNewNodeToExistedGroup(
Review Comment:
I've added the javadoc. Please take a look again, thanks!
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1003935554
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -127,6 +133,8 @@ public RatisConsensus(ConsensusConfig config, IStateMachine.Registry registry)
properties, Collections.singletonList(new File(config.getStorageDir())));
GrpcConfigKeys.Server.setPort(properties, config.getThisNodeEndPoint().getPort());
+ addExecutor = Executors.newSingleThreadExecutor(new IoTThreadFactory("ratis-add"));
Review Comment:
```suggestion
addExecutor = IoTDBThreadPoolFactory.newCachedThreadPool("ratis-add");
```
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -463,6 +484,38 @@ public ConsensusGenericResponse changePeer(ConsensusGroupId groupId, List<Peer>
return ConsensusGenericResponse.newBuilder().setSuccess(reply.isSuccess()).build();
}
+ @Override
+ public ConsensusGenericResponse addNewNodeToExistedGroup(
+ ConsensusGroupId groupId, Peer newNode, List<Peer> originalGroup) {
+
+ CompletableFuture<ConsensusGenericResponse> addResp =
+ CompletableFuture.supplyAsync(() -> addPeer(groupId, newNode), addExecutor);
+
+ try {
+ Thread.sleep(500);
Review Comment:
```suggestion
TimeUnit.MILLISECONDS.sleep(500);
```
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] Beyyes commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
Beyyes commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1004038554
##########
consensus/src/main/java/org/apache/iotdb/consensus/IConsensus.java:
##########
@@ -106,6 +107,17 @@ public interface IConsensus {
*/
ConsensusGenericResponse changePeer(ConsensusGroupId groupId, List<Peer> newPeers);
+ /** Call this method on any member of originalGroup */
+ default ConsensusGenericResponse addNewNodeToExistedGroup(
Review Comment:
Add more comments like `createPeer` and `addPeer`?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #7712:
URL: https://github.com/apache/iotdb/pull/7712#discussion_r1003945327
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/RatisConsensus.java:
##########
@@ -127,6 +133,8 @@ public RatisConsensus(ConsensusConfig config, IStateMachine.Registry registry)
properties, Collections.singletonList(new File(config.getStorageDir())));
GrpcConfigKeys.Server.setPort(properties, config.getThisNodeEndPoint().getPort());
+ addExecutor = Executors.newSingleThreadExecutor(new IoTThreadFactory("ratis-add"));
Review Comment:
Should we use cached thread pool here? Or just a single thread pool?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 merged pull request #7712: [IOTDB-4742] [Consensus] add new api: addNewNodeToExistedGroup
Posted by GitBox <gi...@apache.org>.
JackieTien97 merged PR #7712:
URL: https://github.com/apache/iotdb/pull/7712
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org