zhijiangW commented on a change in pull request #7199: [FLINK-10662][network]
Refactor the ChannelSelector interface for single selected channel
URL: https://github.com/apache/flink/pull/7199#discussion_r241289363
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/api/writer/RecordWriter.java
##########
@@ -68,20 +68,29 @@
private final boolean flushAlways;
+ private final boolean isBroadcastSelector;
+
private Counter numBytesOut = new SimpleCounter();
private Counter numBuffersOut = new SimpleCounter();
public RecordWriter(ResultPartitionWriter writer) {
- this(writer, new RoundRobinChannelSelector<T>());
+ this(writer, new RoundRobinChannelSelector<T>(), false);
}
- @SuppressWarnings("unchecked")
- public RecordWriter(ResultPartitionWriter writer, ChannelSelector<T>
channelSelector) {
- this(writer, channelSelector, false);
+ public RecordWriter(
Review comment:
To be honest, I am also a bit embarrassed for current implementation. It
does exist the redundant and potential inconsistent issues as you mentioned.
The first way you mentioned can solve above issues if we can check whether
the selector is broadcast or not internally in `RecordWriter`.
For the second way we can not only pass the boolean argument without
selector, because once the boolean value is false, we should rely on the
specific selector to get channels. So the selector has to be given explicitly,
but the boolean can be got implicitly from the selector.
I also thought of another way before, that is defining another
BroadcastRecordWriter for handling directly. Then the writer is distinguished
by common `RecordWriter` and special `BroadcastRecordWriter`. In common
`RecordWriter`, the selector is needed for routing channels, and for
`BroadcastRecordWriter` it does not need the selector at all. To do so we may
need further extract the common codes for these two writers, furthermore we may
need to remove current `StreamRecordWriter` which would be integrated with
current `RecordWriter` directly. So the writer is divided by broadcast mode,
not by stream mode. What do you think this way?
I would also further consider the changes for above two options through.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
