[GitHub] pnowojski commented on a change in pull request #7199: [FLINK-10662][network] Refactor the ChannelSelector interface for single selected channel

[GitBox <gi...@apache.org>]

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

 ##########
 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:
   Ops, right 2nd option was pretty stupid on my part 😳 
   
   Isn't having separate optimised `BroadcastRecordWriter` our goal here and whole point of your interest in this area? From this perspective won't this if check:
   ```
    	public void emit(T record) throws IOException, InterruptedException {
    		if (isBroadcastSelector) {
    			broadcastEmit(record);
    		} else {
    			emit(record, channelSelector.selectChannels(record));
    		}
    	}
   ```
   become a dead code/disappear shortly?
   
   In that case indeed, maybe indeed the best way to solve this issue it so actually introduce simple `BroadcastRecordWriter` (which would extend `RecordWriter` and overwrite only one method
   ```
   	@Override 
   	public void emit(T record) throws IOException, InterruptedException {
   		broadcastEmit(record, broadcastChannels);
   	}
   ```
   The only tricky part would be that that would require us to create either some builder method (or even `RecordWriterBuilder` class) something like:
   ```
   public static RecordWriter createRecordWriter(ResultPartitionWriter writer, ChannelSelector<T> channelSelector) {
     // might return either BroadcastRecordWriter or RecordWriter based on passed channelSelector
   }
   ```
   ?
   
   After that the next step would be to optimize `BroadcastRecordWriter`.
   
   What do you think? Does it make sense or have I missed something again 😳?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services