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-1116. Add DataStreamType. #238
Conversation
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. |
ratis-common/src/main/java/org/apache/ratis/datastream/SupportedDataStreamType.java
Show resolved
Hide resolved
.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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newClientFactory -> newServerFactory ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It should be newServerFactory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@szetszwo Thanks the patch. I have merged it. |
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 this is a better test.
* RATIS-1116. Add DataStreamType. * Fix a bug and change the cast(..) method to newInstance(..).
See https://issues.apache.org/jira/browse/RATIS-1116