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.

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

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

DJSagarAhire
Copy link
Contributor

This PR adds an initialization to the dq_failure transitory variable used in the send-to-error Wrangler directive. Without this initialization, if the dq_failure variable is used in Wrangler without it ever being incremented, the directive fails as the variable is not found.

@DJSagarAhire DJSagarAhire added the build Triggers unit test build label Feb 12, 2024
@DJSagarAhire DJSagarAhire self-assigned this Feb 12, 2024
Copy link
Member

@itsankit-google itsankit-google left a 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);
Copy link
Member

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.

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.

@iam-divya iam-divya self-requested a review February 13, 2024 12:25
Copy link
Member

@itsankit-google itsankit-google left a 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:

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?

@DJSagarAhire
Copy link
Contributor Author

@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.

@itsankit-google
Copy link
Member

itsankit-google commented Feb 14, 2024

@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 SendToErrorAndContinue directive is used. Otherwise the same goes with dq_total variable if without using SendToErrorAndContinue directive, the recipe tries to access dq_total variable they can still get a NPE.

@itsankit-google
Copy link
Member

@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.

Also I didn't understand how is this condition brittle and depending on subsequently used directive, can you please give an example?

@DJSagarAhire
Copy link
Contributor Author

@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.

Also I didn't understand how is this condition brittle and depending on subsequently used directive, can you please give an example?

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.

@DJSagarAhire DJSagarAhire changed the title [PLUGIN-1736] Initialize dq_failure before use in send-to-error directive [PLUGIN-1736] Initialize dq_failure before use in send-to-error-and-continue directive Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Triggers unit test build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants