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:
[email protected]
