Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add open() API with FileStatus #1244

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Take CR comments
arunkumarchacko committed Sep 9, 2024
commit 2349976c9dda4c5172ab846d15337303e0fd2efa
Original file line number Diff line number Diff line change
@@ -592,6 +592,13 @@ public FSDataInputStream open(Path hadoopPath, int bufferSize) throws IOExceptio
public FSDataInputStream open(FileStatus status) throws IOException {
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
singhravidutt marked this conversation as resolved.
Show resolved Hide resolved
logger.atFine().log("openWithStatus(%s)", status);

if (!GoogleHadoopFileStatus.class.isAssignableFrom(status.getClass())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInstanceOf would have sufficed, no?

throw new IllegalArgumentException(
String.format(
"Expected status to be of type GoogleHadoopFileStatus, but found %s",
status.getClass()));
}

GoogleHadoopFileStatus fileStatus = (GoogleHadoopFileStatus) status;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be casting issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caller will have to handle the exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to validate and throw IllegalArgument instead of throwing type cast exception from API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


checkPath(status.getPath());
Original file line number Diff line number Diff line change
@@ -274,8 +274,7 @@ public void testTotalTimeStatistics() throws IOException {
}

@Test
@SuppressWarnings("CheckReturnValue")
public void testStatus() throws Exception {
public void testFileOpenWithStatus() throws Exception {
URI bucketName = new URI("gs://read-test-bucket/");
URI failureBucketName = new URI("gs://read-test-bucket-other/");

@@ -296,19 +295,31 @@ public void testStatus() throws Exception {
GoogleHadoopFileStatus fileStatus =
new GoogleHadoopFileStatus(
fileInfo, new Path(fileInfo.getPath()), 1, 2, FsPermission.getFileDefault(), "foo");
new GoogleHadoopFileSystem();
GoogleHadoopFileSystem fs = new GoogleHadoopFileSystem();
fs.initialize(bucketName, new Configuration());
fs.open(fileStatus);

fs.initialize(failureBucketName, new Configuration());

IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> fs.open(fileStatus));
assertThat(exception.getMessage())
.isEqualTo(
"Wrong bucket: read-test-bucket, in path: gs://read-test-bucket/bar/test/object, expected bucket: read-test-bucket-other");
fs.close();
try (GoogleHadoopFileSystem fs = new GoogleHadoopFileSystem()) {
fs.initialize(bucketName, new Configuration());
fs.open(fileStatus);

fs.initialize(failureBucketName, new Configuration());

IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> fs.open(fileStatus));
assertThat(exception.getMessage())
.isEqualTo(
"Wrong bucket: read-test-bucket, in path: gs://read-test-bucket/bar/test/object, expected bucket: read-test-bucket-other");
}
}

@Test
public void testFileOpenWithStatusInvalidType() throws Exception {
try (GoogleHadoopFileSystem fs = new GoogleHadoopFileSystem()) {
fs.initialize(new URI("gs://read-test-bucket/"), new Configuration());

IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> fs.open(new FileStatus()));
assertThat(exception.getMessage())
.isEqualTo(
"Expected status to be of type GoogleHadoopFileStatus, but found class org.apache.hadoop.fs.FileStatus");
}
}

// -----------------------------------------------------------------