[PLUGIN-1837] Error management for Analytics plugin i.e. GroupByAggregate, Deduplicate, Distinct and Joiner #1903
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Amit-CloudSufi
force-pushed
the
hydratorPluginErrorMang
branch
from
c0f275e to
3b3b46f
Compare
psainics
changed the title
Amit-CloudSufi
force-pushed
the
hydratorPluginErrorMang
branch
2 times, most recently
from
2cb74cb to
60c6dee
Compare
| String error = String.format("Invalid filter operation. It should be of format " + | ||
| "'fieldName:functionName'. But got : %s", filterOperation); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| error, error, ErrorType.USER, false, new IllegalArgumentException(error)); |
There was a problem hiding this comment.
no need of wrapping it in IllegalArgumentException, cause can just be null.
This comment applies to whole PR.
There was a problem hiding this comment.
How do you decide when to wrap vs keeping cause null
There was a problem hiding this comment.
If there is no exception being propagated, we don't need to introduce a separation exception here, cause can just be null. In other cases, we are catching the exception and then wrapping it in ProgramFailureException, hence it should be added as a cause.
psainics
force-pushed
the
hydratorPluginErrorMang
branch
from
ffefd24 to
8c11489
Compare
| throw new IllegalStateException("Unsupported condition type " + conditionType); | ||
| String error = String.format("Unsupported condition type %s.", conditionType); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| error, error, ErrorType.SYSTEM, false, null); |
There was a problem hiding this comment.
Unsupported condition can be USER error.
Comment on lines
415
to
421
| String errorMessage = String.format( | ||
| "Invalid aggregate %s(%s): Field '%s' does not exist in input schema %s.", | ||
| functionInfo.getFunction(), functionInfo.getField(), functionInfo.getField(), inputSchema)); | ||
| functionInfo.getFunction(), functionInfo.getField(), functionInfo.getField(), inputSchema); | ||
| String errorReason = String.format("Field '%s' does not exist in input schema %s.", | ||
| functionInfo.getField(), inputSchema); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorReason, errorMessage, ErrorType.USER, false, null); |
There was a problem hiding this comment.
both errorMessage & errorReason are same:
String errorMessage = String.format("Field '%s' does not exist in input schema %s.", functionInfo.getField(), inputSchema);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), errorMessage, errorMessage, ErrorType.USER, false, null);
There was a problem hiding this comment.
updated for whole PR
Comment on lines
175
to
180
| String errorReason = String.format("Field %s does not exist in input schema %s.", | ||
| fieldName, inputSchema); | ||
| String errorMessage = String.format("Failed to fetch record schema due to field %s does not exist in" + | ||
| " input schema %s.", fieldName, inputSchema); | ||
| throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN), | ||
| errorReason, errorMessage, ErrorType.USER, false, null); |
There was a problem hiding this comment.
similar comment here.
String errorMessage = String.format("Failed to fetch record schema due to field %s does not exist in" +
" input schema %s.", fieldName, inputSchema);
throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategory.ErrorCategoryEnum.PLUGIN),
errorMessage, errorMessage, ErrorType.USER, false, null);
Comment on lines
175
to
176
| String errorMessage = String.format( | ||
| "Failed to fetch function due to invalid function '%s', must be one of %s with message: %s.", |
There was a problem hiding this comment.
String errorMessage = String.format("Failed to fetch function due to invalid function '%s' with message: %s, must be one of %s.",
This comment applies to whole PR.
There was a problem hiding this comment.
updated at both places
itsankit-google
approved these changes
Jan 14, 2025
|
Please squash commits before merge |
…ate, Distinct, and Joiner plugins
Amit-CloudSufi
force-pushed
the
hydratorPluginErrorMang
branch
from
01473f2 to
11f1ecf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira : PLUGIN-1837
Test [ Distinct Aggregator ]
Raw Logs
POST v3/namespaces/{namespace-id}/apps/{app-id}/workflows/DataPipelineWorkflow/runs/{run-id}/classify
[ { "stageName": "Distinct", "errorCategory": "Plugin-'Validation'-'Distinct'", "errorReason": "Stage 'Distinct' encountered 1 validation failures.", "errorMessage": "Stage 'Distinct' encountered : io.cdap.cdap.etl.api.validation.ValidationException: Errors were encountered during validation. Field age_fake does not exist in input schema.", "errorType": "USER", "dependency": "false" } ]