-
Notifications
You must be signed in to change notification settings - Fork 56
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-1856] Error management for Wrangler plugin #726
base: develop
Are you sure you want to change the base?
[PLUGIN-1856] Error management for Wrangler plugin #726
Conversation
df84fa4
to
b70d696
Compare
f48259e
to
97fa44c
Compare
wrangler-transform/src/main/java/io/cdap/wrangler/WranglerUtil.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/WranglerUtil.java
Outdated
Show resolved
Hide resolved
a54195a
to
d46f401
Compare
wrangler-transform/src/main/java/io/cdap/wrangler/Precondition.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
335bb68
to
1e04c2d
Compare
wrangler-transform/src/main/java/io/cdap/wrangler/WranglerUtil.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Precondition.java
Outdated
Show resolved
Hide resolved
be7d0f8
to
645593d
Compare
wrangler-transform/src/main/java/io/cdap/wrangler/Precondition.java
Outdated
Show resolved
Hide resolved
String errorMessage = String.format("Precondition '%s' does not result in true or false.", | ||
condition); | ||
throw WranglerUtil.getProgramFailureExceptionDetailsFromChain(null, errorMessage, | ||
ErrorType.USER, false); |
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 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.
I think, user entered the wrong precondition, let me know if you think it can be system error?
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 not handle it at WranglerErrorUtil
level?
For all DirectiveParseException, DirectiveLoadException, RecipeException & PreconditionException error type can be 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.
At few places, we have System error , that's why have made this method generic. If we put null
at error type, by default it will be set to USER
in WranglerErrorUtil class.
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide 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.
Please Fix Unit Tests
fa9723f
to
b7da1fe
Compare
E2E Fixed in #727 ! |
06a92fa
to
ff4f0d5
Compare
@@ -34,7 +35,7 @@ public void testPrecondition() throws Exception { | |||
Assert.assertEquals(false, new Precondition("false").apply(row)); | |||
} | |||
|
|||
@Test(expected = PreconditionException.class) | |||
@Test(expected = ProgramFailureException.class) |
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 match error message to verify we are catching the correct error !
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.
done matched
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.
We are still not matching errorMessage
, only matching Exception
class, 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.
The above expected = ProgramFailureException
is not needed if you are already catching exception in the test.
String.format("Stage:%s - Format of output schema specified is invalid. Please check the format.", | ||
context.getStageName()), e | ||
); | ||
String errorMessage = String.format("Error in stage '%s'. Format of output schema specified " |
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 don't use Error in stage
anywhere.
stageName
is already present in errorCategory
.
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
/** | ||
* Util file to handle exceptions caught in Wrangler plugin | ||
*/ | ||
public class WranglerUtil { |
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.
make this class final.
rename it to WranglerErrorUtil
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
"Error in stage '%s'. Please check the configuration or input data. %s: %s", | ||
context.getStageName(), e.getClass().getName(), e.getMessage()); | ||
throw WranglerUtil.getProgramFailureExceptionDetailsFromChain(e, errorMessage, | ||
ErrorType.SYSTEM, false); |
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
error?
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
if (t instanceof DirectiveLoadException || t instanceof DirectiveParseException | ||
|| t instanceof RecipeException || t instanceof PreconditionException) { | ||
return getProgramFailureException((Exception) t, | ||
errorType != null ? errorType : ErrorType.USER, false); |
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.
based on exception, there can be different subCategory
like Parsing-Directive
, Loading-Directive
, Executing-Recipe
& Precondition
wrangler-transform/src/main/java/io/cdap/wrangler/Precondition.java
Outdated
Show resolved
Hide resolved
@@ -276,8 +286,11 @@ && checkPreconditionNotEmpty(false)) { | |||
} | |||
|
|||
} catch (Exception e) { | |||
LOG.error(e.getMessage()); | |||
collector.addFailure("Error occurred : " + e.getMessage(), null).withStacktrace(e.getStackTrace()); | |||
LOG.error("Error occurred during configuration of the plugin, {}: {}", 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.
be specific in error message what configuration
are we referring to here?
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide 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.
Lots of basic issues in the PR. PTAL, thanks!
f69a84e
to
8231144
Compare
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 add evidences/screenshots of all four known exceptions being captured properly in ErrorManagement UI.
Parsing-Directive, Loading-Directive, Executing-Recipe & Precondition
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
877c7c6
to
32f25df
Compare
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/Wrangler.java
Outdated
Show resolved
Hide resolved
wrangler-transform/src/main/java/io/cdap/wrangler/WranglerErrorUtil.java
Outdated
Show resolved
Hide resolved
collector.addFailure("Error occurred : " + e.getMessage(), null).withStacktrace(e.getStackTrace()); | ||
} | ||
|
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.
nit: remove empty line
wrangler-transform/src/main/java/io/cdap/wrangler/WranglerErrorUtil.java
Show resolved
Hide resolved
e45e426
to
57a3396
Compare
if (t instanceof DirectiveLoadException) { | ||
return getProgramFailureException((DirectiveLoadException) t, | ||
errorType != null ? errorType : ErrorType.USER, false, "Loading-Directive"); | ||
} | ||
if (t instanceof DirectiveParseException) { | ||
return getProgramFailureException((DirectiveParseException) t, | ||
errorType != null ? errorType : ErrorType.USER, false, "Parsing-Directive"); | ||
} | ||
if (t instanceof RecipeException) { | ||
return getProgramFailureException((RecipeException) t, | ||
errorType != null ? errorType : ErrorType.USER, false, "Executing-Recipe"); | ||
} | ||
if (t instanceof PreconditionException) { | ||
return getProgramFailureException((PreconditionException) t, | ||
errorType != null ? errorType : ErrorType.USER, false, "Precondition"); | ||
} |
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 are few more known exceptions like DirectiveNotFoundException
, DirectiveExecutionException
https://cdap.atlassian.net/browse/PLUGIN-1856