liuml07 commented on a change in pull request #1761: HADOOP-16759. Filesystem
openFile() builder to take a FileStatus param
URL: https://github.com/apache/hadoop/pull/1761#discussion_r359123606
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ARemoteFileChanged.java
##########
@@ -432,6 +433,42 @@ public void testReadFileChangedOutOfSyncMetadata() throws
Throwable {
}
}
+ /**
+ * Verifies that when the openFile builder is passed in a status,
+ * then that is used to eliminate the getFileStatus call in open();
+ * thus the version and etag passed down are still used.
+ */
+ @Test
+ public void testOpenFileWithStatus() throws Throwable {
+ final Path testpath = path("testOpenFileWithStatus.dat");
+ final byte[] dataset = TEST_DATA_BYTES;
+ S3AFileStatus originalStatus =
+ writeFile(testpath, dataset, dataset.length, true);
+
+ // forge a file status with a different tag
+ S3AFileStatus forgedStatus =
+ S3AFileStatus.fromFileStatus(originalStatus, Tristate.FALSE,
+ originalStatus.getETag() + "-fake",
+ originalStatus.getVersionId() + "");
+ fs.getMetadataStore().put(
+ new PathMetadata(forgedStatus, Tristate.FALSE, false));
+
+ // By passing in the status open() doesn't need to check s3guard
+ // And hence the existing file is opened
+ try (FSDataInputStream instream = fs.openFile(testpath)
+ .withFileStatus(originalStatus)
+ .build().get()) {
+ instream.read();
+ }
+
+ // and this holds for S3A Located Status
+ try (FSDataInputStream instream = fs.openFile(testpath)
+ .withFileStatus(new S3ALocatedFileStatus(originalStatus, null))
+ .build().get()) {
+ instream.read();
+ }
+ }
Review comment:
The two try-with tests are testing that with a good FileStatus passed, it's
able to skip the S3Guard and hence no errors are reported regardless of change
detection policy.
I was thinking is it worth to make sure if we don't pass file status, the
forged file status is indeed used. So it will error out because of bad etag.
For e.g.
```
try(FSDataInputStream instream = fs.openFile(testpath)
.build().get()) {
try {
instream.read();
// No exception only if we don't enforce change detection as exception
assertTrue(changeDetectionMode.equals(CHANGE_DETECT_MODE_NONE) ||
changeDetectionMode.equals(CHANGE_DETECT_MODE_WARN));
} catch (Exception ignored) {
// Ignored.
}
}
```
I think again and now guess this extra test perhaps is not required to
demonstrate the withFileStatus is being honored.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
