Fix a bug that the file copied by TF from HDFS to local may be wrong,…

[yuanbopeng]

This is a PR from TaiJi AI platform in Tencent.

• The file copied by TF from HDFS to local may be wrong, when HDFS file is being overwritten #42597

[yuanbopeng]

@mihaimaruseac
Can you help review the patch-3? I look forward to your comments, thanks :).

[mihaimaruseac]

Not the ideal patch, but at least this is localized to the HDFS implementation and does not remove existing tests.

Would be good if you can also add a test and add a similar change to the existing filesystem plugin for HDFS.

[vnghia]

I will take care for the change and test in filesystem plugin for HDFS. However, I prefer reading the env in the initialization of the filesystem for a consistency between multiple calls for hdfsRead. what do you think @yuanbopeng @mihaimaruseac

[yuanbopeng]

@vnvo2409
I will add a test case for ReadWhileOverwriting soon, and I will also fix the compilation problem caused by the HadoopFileSystemTest call HDFS API without TransactionTokend. S3FileSystemTest also has similar problems.

If it is necessary to modify, I prefer to reading the env in the initialization of HDFSRandomAccessFile, which is more flexible and can support both WriteWhileReading and ReadWhileOverwriting use cases.

[vnghia]

@yuanbopeng

• The filesystem plugin is something else. It lives here https://github.com/tensorflow/tensorflow/tree/master/tensorflow/c/experimental/filesystem/plugins/hadoop. The current filesystem will be replaced by the filesystem plugin but it will take some time. In the meantime, I think you could just work on the current filesystem and I will mimic your change to the filesystem plugins.
• So i will read the env inside NewRandomAccessFile for Hadoop.

[yuanbopeng]

@vnvo2409

• The filesystem plugin is something else. It lives here https://github.com/tensorflow/tensorflow/tree/master/tensorflow/c/experimental/filesystem/plugins/hadoop. The current filesystem will be replaced by the filesystem plugin but it will take some time. In the meantime, I think you could just work on the current filesystem and I will mimic your change to the filesystem plugins.

ok. In order to ensure that the WriteWhileReading test case can pass, I will delete test cases that caused the compilation problem locally, and finally submit only the WriteWhileReading test case code.

• So i will read the env inside NewRandomAccessFile for Hadoop.

Would it be better to read env in the constructor of HDFSRandomAccessFile?

[vnghia]

@yuanbopeng

Would it be better to read env in the constructor of HDFSRandomAccessFile?

Agree.

ok. In order to ensure that the WriteWhileReading test case can pass, I will delete test cases that caused the compilation problem locally, and finally submit only the WriteWhileReading test case code.

The cloud filesystems tests have a tag manual so the CI will skip them. You see the compilation problem because no one has run those tests for a long time while adding a new transactional token into the filesystem operation but not into the tests. I think it is not necessary adding the test here since you will have to add the transactional token ( a nullptr I think so ) to every functions to fix the compilation problem.

I think I will add your change and add my own test into HDFS plugins. I will ping you in that PR so you could review the test and make sure that the test is what you want. The implementation of the filesystem plugin and the current filesystem are very similar so I think adding test in one place is enough.

You could also work with the filesystem plugin but keep in mind that if you would like to see the change immediately (tf-nightly). You will have to work with the current filesystem.

let's see if @mihaimaruseac agree with us.

[mihaimaruseac]

Sounds good to me. Thanks for driving this forward

[yuanbopeng]

@vnvo2409
add ReadWhileOverwriting test cases. #43220

In addition, fixes the compilation problem.
HDFS: #43221
S3: #43222