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.

Sign up for GitHub

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

Jump to bottom

Add java-storage implementation for createBucket. #1074

Merged
merged 4 commits into from
Nov 23, 2023
Merged

Add java-storage implementation for createBucket. #1074

merged 4 commits into from
Nov 23, 2023

Conversation

shilpi23pandey
Copy link
Contributor

No description provided.

@shilpi23pandey shilpi23pandey marked this pull request as draft November 16, 2023 10:02
@shilpi23pandey shilpi23pandey marked this pull request as ready for review November 17, 2023 06:17
@arunkumarchacko
Copy link
Contributor

/gcbrun
@arunkumarchacko
Copy link
Contributor

/gcbrun
@arunkumarchacko
Copy link
Contributor

/gcbrun
@arunkumarchacko
Copy link
Contributor

/gcbrun
1 similar comment
@arunkumarchacko
Copy link
Contributor

/gcbrun
@shilpi23pandey
Fix FileAlreadyExists exception condition.
abd8db4 
gRPC API throws a FAILED_PRECONDITION status code for bucket already exists condition on create bucket as opposed to the JSON API.
arunkumarchacko
arunkumarchacko reviewed Nov 23, 2023
View reviewed changes
util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java Outdated
@Override
public boolean bucketAlreadyExists(Exception e) {
ErrorType errorType = getErrorType(e);
if (errorType == ErrorType.ALREADY_EXISTS) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please avoid single line if statements.
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
util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java Outdated
@@ -25,6 +27,9 @@ public class GrpcErrorTypeExtractor implements ErrorTypeExtractor {

public static final GrpcErrorTypeExtractor INSTANCE = new GrpcErrorTypeExtractor();

public static final String BUCKET_ALREADY_EXISTS_MESSAGE =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be private?
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
util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java Outdated
@Nullable
private StatusRuntimeException getStatusRuntimeException(Exception e) {
Throwable cause = e;
while (cause != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this go into an infinite loop?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, Added check for self-reference so it doesnt go into infinite loop.
util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java
else if (errorType == ErrorType.FAILED_PRECONDITION) {
StatusRuntimeException statusRuntimeException = getStatusRuntimeException(e);
return statusRuntimeException != null
&& BUCKET_ALREADY_EXISTS_MESSAGE.equals(statusRuntimeException.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an error code? Checking for error message is not fool proof. Can it return different error message based on caller locale?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are checking the status code on line 58, but it is a generic FAILED_PRECONDITION code so added another level of check via error message. I dont think it returns different message based on caller locale as we have similar message check for json api as well -

hadoop-connectors/util/src/main/java/com/google/cloud/hadoop/util/ApiErrorExtractor.java

Line 224 in fabf01d

&& USER_PROJECT_MISSING_MESSAGE.equals(jsonError.getMessage());

@arunkumarchacko
Copy link
Contributor

/gcbrun
arunkumarchacko
arunkumarchacko reviewed Nov 23, 2023
View reviewed changes
util/src/main/java/com/google/cloud/hadoop/util/GrpcErrorTypeExtractor.java Outdated
return (StatusRuntimeException) cause;
}
cause = cause.getCause();
} while (cause != null && cause != e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will catch only self loops, correct? Why not have a counter of say 1000 and break after that.
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
@arunkumarchacko
Copy link
Contributor

/gcbrun
@arunkumarchacko
Copy link
Contributor

/gcbrun
@arunkumarchacko arunkumarchacko merged commit 3ab7546 into GoogleCloudDataproc:master Nov 23, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants
@shilpi23pandey @arunkumarchacko