XComp commented on code in PR #24390:
URL: https://github.com/apache/flink/pull/24390#discussion_r1505770714
##########
flink-connectors/flink-connector-files/src/test/java/org/apache/flink/connector/file/table/FileSystemOutputFormatTest.java:
##########
Review Comment:
I guess, I'm not the right person to judge here because the connector
codebase is not really an area where I have much knowledge. But the test you
added doesn't cover the fix (neither does reverting the fix of this PR make any
of the newly added tests fail nor does any of the tests call the method in
question `FileSystemTableSink#toStagingPath`).
That is where I'm wondering what the value of the added tests are. Or am I
missing something here? :thinking:
##########
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/table/FileSystemTableSink.java:
##########
@@ -374,7 +374,10 @@ public DynamicTableSource.DataStructureConverter
createDataStructureConverter(
}
private Path toStagingPath() {
- Path stagingDir = new Path(path, ".staging_" +
System.currentTimeMillis());
+ // Add a random UUID to prevent multiple sinks from sharing the same
staging dir.
+ // Please see FLINK-29114 for more details
+ Path stagingDir =
+ new Path(path, ".staging_" + UUID.randomUUID() +
System.currentTimeMillis());
Review Comment:
```suggestion
new Path(path, ".staging_" + UUID.randomUUID());
```
Maybe, you missed [my previous
comment](https://github.com/apache/flink/pull/24390/files#r1503917055) but
shouldn't we be able to only use the UUID here?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
