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