-
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-1736] Initialize dq_failure before use in send-to-error-and-continue directive #700
Conversation
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.
JIRA also mentions another issue: Even though send-to-error condition is false, record was send to error port.
Please add unit tests.
@@ -127,6 +127,7 @@ public List<Row> execute(List<Row> rows) throws RecipeException { | |||
// Resets the scope of local variable. | |||
if (context != null) { | |||
context.getTransientStore().reset(TransientVariableScope.LOCAL); | |||
context.getTransientStore().set(TransientVariableScope.LOCAL, "dq_failure", 0L); |
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 a comment why this variable is being initialized 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.
Done.
wrangler-core/src/main/java/io/cdap/wrangler/executor/RecipePipelineExecutor.java
Outdated
Show resolved
Hide resolved
3cb4bd0
to
9a8ced0
Compare
9a8ced0
to
91674ec
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.
Why can't we add an else condition here:
wrangler/wrangler-core/src/main/java/io/cdap/directives/row/SendToErrorAndContinue.java
Lines 117 to 128 in ddff013
if (result.getBoolean()) { | |
if (metric != null && context != null) { | |
context.getMetrics().count(metric, 1); | |
} | |
if (message == null) { | |
message = condition; | |
} | |
if (context != null) { | |
context.getTransientStore().increment(TransientVariableScope.LOCAL, "dq_failure", 1); | |
} | |
throw new ReportErrorAndProceed(message, 1); | |
} |
And check if the variable dq_failure
does not exist then initialize it?
@itsankit-google I feel it's better to initialize the variable at the very beginning where it will be picked up regardless of what happens in subsequently used directives rather than rely on some potentially brittle condition. Lmk if you agree. |
According to the documentation, we don't expect the variable to be used unless |
Also I didn't understand how is this condition brittle and depending on subsequently used directive, can you please give an example? |
91674ec
to
5cadb36
Compare
Never mind, I figured that referring to the variables without using Send-To-Error-And-Continue is an error anyway, so I made the change. |
This PR adds an initialization to the
dq_failure
transitory variable used in the send-to-error Wrangler directive. Without this initialization, if thedq_failure
variable is used in Wrangler without it ever being incremented, the directive fails as the variable is not found.