xintongsong commented on code in PR #25226:
URL: https://github.com/apache/flink/pull/25226#discussion_r1764322519
##########
flink-core/src/main/java/org/apache/flink/core/fs/FileSystem.java:
##########
@@ -665,6 +666,16 @@ public RecoverableWriter createRecoverableWriter() throws
IOException {
return IFileSystem.super.createRecoverableWriter();
}
+ @PublicEvolving
+ @Override
+ public RecoverableWriter createRecoverableWriter(boolean noLocalWrite)
throws IOException {
Review Comment:
I'd suggest to add a method `createRecoverbleWriter(Map<String, String>
conf)`, and introduce some String constants for the config keys. This helps
avoid introducing a new overloading `createRecoverableWriter(x, y, zzz)` every
time we need a new parameter.
##########
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/sink/FileSink.java:
##########
@@ -580,6 +582,11 @@ public T withRollingPolicy(CheckpointRollingPolicy<IN,
String> rollingPolicy) {
return self();
}
+ public T enableNoLocalWriting() {
Review Comment:
I'd suggest the name `disableLocalWrite`.
##########
flink-filesystems/flink-hadoop-fs/src/test/java/org/apache/flink/runtime/fs/hdfs/HadoopRecoverableWriterTest.java:
##########
@@ -83,6 +86,18 @@ static void destroyHDFS() throws Exception {
}
}
+ private RecoverableWriter getNoLocalWriteFileSystemWriter() throws
Exception {
+ return fileSystem.createRecoverableWriter(true);
+ }
+
+ @Test
+ void testNoLocalWrite() throws Exception {
+ final HadoopRecoverableWriter writer =
+ (HadoopRecoverableWriter) getNoLocalWriteFileSystemWriter();
+
+ Assertions.assertTrue(writer.noLocalWrite);
+ }
Review Comment:
I think we should verify that the data is actually not written to the local
path. Currently, the test case only verifies that the internal field is set
correctly, but doesn't cover the code path of leveraging the internal field and
change the write behavior.
##########
flink-filesystems/flink-hadoop-fs/src/main/java/org/apache/flink/runtime/fs/hdfs/HadoopRecoverableWriter.java:
##########
@@ -46,13 +46,22 @@ public class HadoopRecoverableWriter implements
RecoverableWriter {
/** The Hadoop file system on which the writer operates. */
protected final org.apache.hadoop.fs.FileSystem fs;
+ protected final boolean noLocalWrite;
Review Comment:
It seems the only reason this field is `protected` rather than `private` is
that it is used in a test case. This is usually indicates a white-box test,
which should be avoided as much as possible.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
