-
Notifications
You must be signed in to change notification settings - Fork 86
[PLUGIN-1842] Error Management catch known errors [GCSBucketCreate] #1488
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
[PLUGIN-1842] Error Management catch known errors [GCSBucketCreate] #1488
Conversation
| context.getFailureCollector().addFailure(String.format("%s %s", errorReason, e.getMessage()), null) | ||
| .withStacktrace(e.getStackTrace()); | ||
| context.getFailureCollector().getOrThrowException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collector is already defined at line 73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated !
| credentials = serviceAccount == null ? null : GCPUtils.loadServiceAccountCredentials(serviceAccount, | ||
| isServiceAccountFilePath); | ||
| } catch (IOException e) { | ||
| String errorReason = "Failed to load service account credentials. "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove whitespace before period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed !
| isServiceAccountFilePath); | ||
| } catch (IOException e) { | ||
| String errorReason = "Failed to load service account credentials. "; | ||
| context.getFailureCollector().addFailure(String.format("%s %s", errorReason, e.getMessage()), null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.format("%s %s: %s", errorReason, e.getClass().getName(), e.getMessage())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated !
| String errorReason = String.format("Unable to access or create bucket %s. ", gcsPath.getBucket()) + | ||
| "Ensure you entered the correct bucket path and have permissions for it."; | ||
| String errorMessage = String.format("%s %s", errorReason, e.getMessage()); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorReason, errorMessage, ErrorType.USER, true, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StorageException returns http error codes. Handle similarly as we do for GoogleJsonResponseException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw new Exception(String.format("Path %s already exists", gcsPath)); | ||
| String errorReason = String.format("Path %s already exists", gcsPath); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorReason, errorReason, ErrorType.SYSTEM, true, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a system error, it is a user error as bucket name is provided by user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to user error.
| String errorReason = String.format("Path %s already exists", gcsPath); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorReason, errorReason, ErrorType.SYSTEM, true, null); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed !
| String errorReason = "Unable to get GCS filesystem handler."; | ||
| String errorMessage = String.format("%s %s", errorReason, e.getMessage()); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorReason, errorMessage, ErrorType.SYSTEM, false, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a system error, they should be unknown errors.
Also does IOException underneath has GoogleJsonResponse, then we should do as we perform in GCPErrorDetailsProvider.
similar comment for all errors in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to unknown !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the IOException to getHttpResponseExceptionDetailsFromChain
| } | ||
| return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, | ||
| String.format("%s %s: %s", errorReason, e.getClass().getName(), errorMessage), pair.getErrorType(), true, | ||
| ErrorCodeType.HTTP, statusCode.toString(), GCPUtils.GCS_SUPPORTED_DOC_URL, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why GCS_SUPPORTED_DOC_URL always, this should be passed as parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing this in function param now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is still not resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved now !
| rollback = true; | ||
| throw new Exception("Unable to get GCS filesystem handler. " + e.getMessage(), e); | ||
| String errorReason = "Unable to get GCS filesystem handler."; | ||
| throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.SYSTEM, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why SYSTEM, these error type should be UNKNOWN by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment applies to whole PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to unknown errors
| String errorReason = String.format("Failed to create path %s.", gcsPath); | ||
| throw GCPErrorDetailsProviderUtil.getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.SYSTEM, | ||
| true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add evidence of one such instance where it extracts details from IOException and return correct result in Error Management UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't seem to trigger this one.
i theory it should work if there is a HttpResponseException in the cause chain, else the generic exception should be returned.
Test cases i have tried.
- Debug Pause to create the dir but it does not fail if dir alredy exist
- Hardcode in incorrect path, fails during validation / IllegalArgumentException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can add permission to create GCSBucket but do not add permission to create objects it should fail at that step right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it have whole errorReason copied in errorMessage in the screenshot?
src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProviderUtil.java
Show resolved
Hide resolved
itsankit-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also do the same for whenever we are creating storage client like on line 115.
This is being addressed in a separate PR. |
b653d07 to
5141087
Compare
|
Rebased and Squashed |
| private static ProgramFailureException getProgramFailureException(HttpResponseException e, String externalDocUrl) { | ||
| String errorReason = getErrorReason(e, externalDocUrl); | ||
| return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, | ||
| String.format("%s %s: %s", errorReason, e.getClass().getName(), getErrorMessage(e)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not copy errorReason in errorMessage, was it done earlier in GCPErrorDetailsProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i think it;s done in GCPErrorDetailsProvider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it was not like that. Please see old code:
google-cloud/src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProvider.java
Lines 79 to 103 in ee6cb2e
| String errorReason = String.format("%s %s. %s", e.getStatusCode(), e.getStatusMessage(), | |
| pair.getCorrectiveAction()); | |
| String errorMessage = e.getMessage(); | |
| String externalDocumentationLink = null; | |
| if (e instanceof GoogleJsonResponseException) { | |
| errorMessage = getErrorMessage((GoogleJsonResponseException) e); | |
| externalDocumentationLink = getExternalDocumentationLink(); | |
| if (!Strings.isNullOrEmpty(externalDocumentationLink)) { | |
| if (!errorReason.endsWith(".")) { | |
| errorReason = errorReason + "."; | |
| } | |
| errorReason = String.format("%s For more details, see %s", errorReason, | |
| externalDocumentationLink); | |
| } | |
| } | |
| return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN), | |
| errorReason, String.format(ERROR_MESSAGE_FORMAT, errorContext.getPhase(), | |
| e.getClass().getName(), errorMessage), | |
| pair.getErrorType(), true, ErrorCodeType.HTTP, statusCode.toString(), | |
| externalDocumentationLink, e); | |
| } |
itsankit-google
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment. PTAL, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think current implementation of separating out errorReason and errorMessage is not needed, can you just pull this method in GCPErrorDetailsProviderUtil and call it in both places?
private ProgramFailureException getProgramFailureException(HttpResponseException e,
@Nullable ErrorContext errorContext) {
Integer statusCode = e.getStatusCode();
ErrorUtils.ActionErrorPair pair = ErrorUtils.getActionErrorByStatusCode(statusCode);
String errorReason = String.format("%s %s. %s", e.getStatusCode(), e.getStatusMessage(),
pair.getCorrectiveAction());
String errorMessage = e.getMessage();
String externalDocumentationLink = null;
if (e instanceof GoogleJsonResponseException) {
errorMessage = getErrorMessage((GoogleJsonResponseException) e);
externalDocumentationLink = getExternalDocumentationLink();
if (!Strings.isNullOrEmpty(externalDocumentationLink)) {
if (!errorReason.endsWith(".")) {
errorReason = errorReason + ".";
}
errorReason = String.format("%s For more details, see %s", errorReason,
externalDocumentationLink);
}
}
return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN),
errorReason, errorMessage != null ? String.format(ERROR_MESSAGE_FORMAT, errorContext.getPhase(),
e.getClass().getName(), errorMessage) : String.format("%s: %s", e.getClass().getName(), errorMessage),
pair.getErrorType(), true, ErrorCodeType.HTTP, statusCode.toString(),
externalDocumentationLink, e);
}
if errorContext is null you can just keep the errorMessage without adding errorPhase information.
5141087 to
dbba9cd
Compare
| } | ||
| return ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorReason, | ||
| errorContext != null ? | ||
| String.format("Error occurred in the phase: '%s'. %s: %s", errorContext.getPhase(), e.getClass().getName(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can make ERROR_MESSAGE_FORMAT package private in GCPErrorDetailsProvider and use here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated !
|
|
||
| public static ProgramFailureException getHttpResponseExceptionDetailsFromChain(Exception e, String errorReason, | ||
| String externalDocUrl) { | ||
| return getHttpResponseExceptionDetailsFromChain(e, errorReason, ErrorType.USER, true, externalDocUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you want to delegate the call to another method here?
Also errorType should be UNKNOWN by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the errors are user error, i did this to not have to write this again , and directly use this function!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For errors that will be unknown we will specify the error type, this is only to be used with error that are caused due to user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no ProgramFailureException & HttpResponseException in the causal chain, how do we know it is USER?
src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProviderUtil.java
Show resolved
Hide resolved
dbba9cd to
73a0446
Compare
src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProviderUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cdap/plugin/gcp/gcs/actions/GCSBucketCreate.java
Outdated
Show resolved
Hide resolved
cf1e771 to
c22e84d
Compare
src/main/java/io/cdap/plugin/gcp/gcs/actions/GCSBucketCreate.java
Outdated
Show resolved
Hide resolved
c22e84d to
bdd55f3
Compare





Error Management catch known errors
Jira : PLUGIN-1842
Description
Code change
GCSBucketCreate.javaTests
Raw Logs
POST v3/namespaces/{namespace-id}/apps/{app-id}/workflows/DataPipelineWorkflow/runs/{run-id}/classify
[ { "stageName": "GCS Create", "errorCategory": "Plugin-'GCS Create'", "errorReason": "Unable to access or create bucket this_is_not_my_bucket. Ensure you entered the correct bucket path and have permissions for it.", "errorMessage": "Unable to access or create bucket this_is_not_my_bucket. Ensure you entered the correct bucket path and have permissions for it. [email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).", "errorType": "USER", "dependency": "true" } ]