[GitHub] zhijiangW commented on a change in pull request #7438: [FLINK-11282][network] Merge StreamRecordWriter into RecordWriter
zhijiangW commented on a change in pull request #7438: [FLINK-11282][network]
Merge StreamRecordWriter into RecordWriter
URL: https://github.com/apache/flink/pull/7438#discussion_r250035752
##
File path:
flink-streaming-java/src/test/java/org/apache/flink/streaming/runtime/io/benchmark/StreamNetworkPointToPointBenchmark.java
##
@@ -71,7 +71,7 @@ public void setUp(long flushTimeout) throws Exception {
*
* @param flushTimeout
* output flushing interval of the
-* {@link
org.apache.flink.streaming.runtime.io.StreamRecordWriter}'s output flusher
thread
+* {@link
org.apache.flink.runtime.io.network.api.writer}'s output flusher thread
Review comment:
，I would add it in a fixup commit.
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
[GitHub] zhijiangW commented on a change in pull request #7438: [FLINK-11282][network] Merge StreamRecordWriter into RecordWriter
zhijiangW commented on a change in pull request #7438: [FLINK-11282][network] Merge StreamRecordWriter into RecordWriter URL: https://github.com/apache/flink/pull/7438#discussion_r246626579 ## 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: `AtomicReference` can be a final variable which seems more better if not affect performance. 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
[GitHub] zhijiangW commented on a change in pull request #7438: [FLINK-11282][network] Merge StreamRecordWriter into RecordWriter
zhijiangW commented on a change in pull request #7438: [FLINK-11282][network] Merge StreamRecordWriter into RecordWriter URL: https://github.com/apache/flink/pull/7438#discussion_r246626442 ## 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: You pointed out a previous potential risk. I am not sure why the history code did not add `sync/volatile` here, unless the real-time visibility of this variable is not very important for task thread, as long as it can be seen finally. But such behavior still seems undetermined. I suggest adding volatile keyword or changing this variable as `AtomicReference`. Which one do you prefer? 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
