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/11/03 14:04:57 UTC
[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #245: RATIS-1084. Support multiple groups in Streaming
amaliujia commented on a change in pull request #245:
URL: https://github.com/apache/incubator-ratis/pull/245#discussion_r516376642
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -94,21 +95,21 @@ void addPeers(Collection<RaftPeer> newPeers) {
peers.addAll(newPeers);
}
- List<DataStreamOutput> getDataStreamOutput() throws IOException {
+ List<DataStreamOutput> getDataStreamOutput(RaftGroupId groupId) throws IOException {
final List<DataStreamOutput> outs = new ArrayList<>();
try {
- getDataStreamOutput(outs);
+ getDataStreamOutput(outs, groupId);
} catch (IOException e) {
outs.forEach(CloseAsync::closeAsync);
throw e;
}
return outs;
}
- private void getDataStreamOutput(List<DataStreamOutput> outs) throws IOException {
+ private void getDataStreamOutput(List<DataStreamOutput> outs, RaftGroupId groupId) throws IOException {
Review comment:
I feel like better not to do so, though it is easy to merge.
The reason is `List<DataStreamOutput> getDataStreamOutput(RaftGroupId groupId)` is a simple API to use for creating `List<DataStreamOutput>` based on group id. Merging these two function means we only offer `private void getDataStreamOutput(List<DataStreamOutput> outs, RaftGroupId groupId)`, then the caller will need to manage this:
```
catch (IOException e) {
outs.forEach(CloseAsync::closeAsync);
throw e;
}
```
There is only one caller so looks fine for now. But if in the future there are more callers I think we will go back to `List<DataStreamOutput> getDataStreamOutput(RaftGroupId groupId)`
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -94,21 +95,21 @@ void addPeers(Collection<RaftPeer> newPeers) {
peers.addAll(newPeers);
}
- List<DataStreamOutput> getDataStreamOutput() throws IOException {
+ List<DataStreamOutput> getDataStreamOutput(RaftGroupId groupId) throws IOException {
final List<DataStreamOutput> outs = new ArrayList<>();
try {
- getDataStreamOutput(outs);
+ getDataStreamOutput(outs, groupId);
} catch (IOException e) {
outs.forEach(CloseAsync::closeAsync);
throw e;
}
return outs;
}
- private void getDataStreamOutput(List<DataStreamOutput> outs) throws IOException {
+ private void getDataStreamOutput(List<DataStreamOutput> outs, RaftGroupId groupId) throws IOException {
Review comment:
I feel like better not to do so, though it is easy to merge.
The reason is `List<DataStreamOutput> getDataStreamOutput(RaftGroupId groupId)` is a simple API to use for creating `List<DataStreamOutput>` based on group id. Merging these two function means we only offer `private void getDataStreamOutput(List<DataStreamOutput> outs, RaftGroupId groupId)`, then the caller will need to manage this:
```
catch (IOException e) {
outs.forEach(CloseAsync::closeAsync);
throw e;
}
```
There is only one caller so looks fine for now. But if in the future there are more callers I think we will go back to `List<DataStreamOutput> getDataStreamOutput(RaftGroupId groupId)` to encapsulate this common logic `outs.forEach(CloseAsync::closeAsync);`
----------------------------------------------------------------
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