pnowojski commented on a change in pull request #7438: [FLINK-11282][network] Merge StreamRecordWriter into RecordWriter URL: https://github.com/apache/flink/pull/7438#discussion_r246713184
########## File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/api/writer/RecordWriter.java ########## @@ -72,17 +73,29 @@ private Counter numBuffersOut = new SimpleCounter(); + /** Default name for teh output flush thread, if no name with a task reference is given. */ + private static final String DEFAULT_OUTPUT_FLUSH_THREAD_NAME = "OutputFlusher"; + + /** The thread that periodically flushes the output, to give an upper latency bound. */ + private final OutputFlusher outputFlusher; + + /** The exception encountered in the flushing thread. */ + private Throwable flusherException; Review comment: I talked about it with @NicoK and we have a suspicion that just best effort error checking here is enough. Adding synchronisation on the other hand would add a synchronisation overhead on the critical path (checking for the error happens per each record emission). So maybe it would be best to leave it as it is, but adding a comment that we are fine here with "error being thrown usually/eventually"? Probably it is a good idea to add a `LOG.error` call to `StreamRecordWriter#notifyFlusherException` to make this error at least always visible. ---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services