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/10/29 11:09:14 UTC

[GitHub] [incubator-ratis] szetszwo opened a new pull request #238: RATIS-1116. Add DataStreamType.

szetszwo opened a new pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238


   See https://issues.apache.org/jira/browse/RATIS-1116


----------------------------------------------------------------
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] szetszwo commented on a change in pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#discussion_r514218490



##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/DataStreamServerImpl.java
##########
@@ -41,15 +41,15 @@ public DataStreamServerImpl(RaftPeer server, StateMachine stateMachine,
       RaftProperties properties, Parameters parameters){
     final SupportedDataStreamType type = RaftConfigKeys.DataStream.type(properties, LOG::info);
 
-    this.serverRpc = DataStreamServerFactory.cast(type.newFactory(parameters))
+    this.serverRpc = DataStreamServerFactory.cast(type.newServerFactory(parameters))
         .newDataStreamServerRpc(server, stateMachine, properties);
   }
 
   public DataStreamServerImpl(RaftServer server, StateMachine stateMachine,
       RaftProperties properties, Parameters parameters){
     final SupportedDataStreamType type = RaftConfigKeys.DataStream.type(properties, LOG::info);
 
-    this.serverRpc = DataStreamServerFactory.cast(type.newFactory(parameters))
+    this.serverRpc = DataStreamServerFactory.cast(type.newClientFactory(parameters))

Review comment:
       Good catch.  It should be newServerFactory.




----------------------------------------------------------------
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 #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#issuecomment-718743126


   @szetszwo Thanks the patch. I have merged it.


----------------------------------------------------------------
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 merged pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238


   


----------------------------------------------------------------
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] szetszwo commented on a change in pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#discussion_r514213541



##########
File path: ratis-common/src/main/java/org/apache/ratis/datastream/SupportedDataStreamType.java
##########
@@ -20,30 +20,40 @@
 import org.apache.ratis.conf.Parameters;
 import org.apache.ratis.util.ReflectionUtils;
 
-public enum SupportedDataStreamType implements DataStreamFactory {
-  DISABLED("org.apache.ratis.server.impl.DisabledDataStreamFactory"),
+public enum SupportedDataStreamType implements DataStreamType {
+  DISABLED("org.apache.ratis.client.DisabledDataStreamClientFactory",
+      "org.apache.ratis.server.DisabledDataStreamServerFactory"),
   NETTY("org.apache.ratis.netty.NettyDataStreamFactory");

Review comment:
       Netty is in ratis-netty which depends on ratis-server and ratis-client.  So splitting it won't be useful at the moment.  It will be useful if we split ratis-netty into ratis-netty-client and ratis-netty-server later on.




----------------------------------------------------------------
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 a change in pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#discussion_r514185916



##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/DataStreamServerImpl.java
##########
@@ -41,15 +41,15 @@ public DataStreamServerImpl(RaftPeer server, StateMachine stateMachine,
       RaftProperties properties, Parameters parameters){
     final SupportedDataStreamType type = RaftConfigKeys.DataStream.type(properties, LOG::info);
 
-    this.serverRpc = DataStreamServerFactory.cast(type.newFactory(parameters))
+    this.serverRpc = DataStreamServerFactory.cast(type.newServerFactory(parameters))
         .newDataStreamServerRpc(server, stateMachine, properties);
   }
 
   public DataStreamServerImpl(RaftServer server, StateMachine stateMachine,
       RaftProperties properties, Parameters parameters){
     final SupportedDataStreamType type = RaftConfigKeys.DataStream.type(properties, LOG::info);
 
-    this.serverRpc = DataStreamServerFactory.cast(type.newFactory(parameters))
+    this.serverRpc = DataStreamServerFactory.cast(type.newClientFactory(parameters))

Review comment:
       newClientFactory -> newServerFactory ?




----------------------------------------------------------------
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] amaliujia commented on a change in pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#discussion_r514475108



##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamDisabled.java
##########
@@ -41,7 +42,8 @@ public void testDataStreamDisabled() throws Exception {
       setupServer();
       setupClient();
       exception.expect(UnsupportedOperationException.class);
-      exception.expectMessage("org.apache.ratis.server.impl.DisabledDataStreamFactory$1 does not support streamAsync");
+      exception.expectMessage(DisabledDataStreamClientFactory.class.getName()

Review comment:
       +1 this is a better test.




----------------------------------------------------------------
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] szetszwo commented on pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#issuecomment-718671962


   I also took this chance to split the newFactory method into newClientFactory and newServerFactory so that the client factory for DISABLE can be moved to the ratis-client module.


----------------------------------------------------------------
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 a change in pull request #238: RATIS-1116. Add DataStreamType.

Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #238:
URL: https://github.com/apache/incubator-ratis/pull/238#discussion_r514184137



##########
File path: ratis-common/src/main/java/org/apache/ratis/datastream/SupportedDataStreamType.java
##########
@@ -20,30 +20,40 @@
 import org.apache.ratis.conf.Parameters;
 import org.apache.ratis.util.ReflectionUtils;
 
-public enum SupportedDataStreamType implements DataStreamFactory {
-  DISABLED("org.apache.ratis.server.impl.DisabledDataStreamFactory"),
+public enum SupportedDataStreamType implements DataStreamType {
+  DISABLED("org.apache.ratis.client.DisabledDataStreamClientFactory",
+      "org.apache.ratis.server.DisabledDataStreamServerFactory"),
   NETTY("org.apache.ratis.netty.NettyDataStreamFactory");

Review comment:
       @szetszwo Do we also need to split NettyDataStreamFactory into client and server ?




----------------------------------------------------------------
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