-
Notifications
You must be signed in to change notification settings - Fork 344
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
[CDAP-21064] Validate workflow token on program status trigger. #15810
base: develop
Are you sure you want to change the base?
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.
So, this just changes NPE into error log, but does not seem to solve the problem of children jobs not starting. We need to ensure we retry the check later.
@@ -209,7 +210,7 @@ private void addNotification(Job job, Notification notification) throws IOExcept | |||
} | |||
|
|||
private boolean isTriggerSatisfied(ProgramSchedule schedule, List<Notification> notifications) { | |||
return ((SatisfiableTrigger) schedule.getTrigger()).isSatisfied(schedule, notifications); | |||
return ((SatisfiableTrigger) schedule.getTrigger()).isSatisfied(schedule, new NotificationContext(notifications,appMetadataStore)); |
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 space after comma
import java.io.IOException; | ||
import java.util.List; | ||
|
||
public class NotificationContext { |
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.
Add javadocs
}); | ||
public boolean isSatisfied(ProgramSchedule schedule, NotificationContext notificationContext) { | ||
return getTriggerSatisfiedResult(notificationContext.getNotifications(), false, | ||
new Function<ProgramRunInfo, Boolean>() { |
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 it a lambda
Adding a workflow token validation for composite program status trigger.
Background
When a run completes it sends 2 messages one for program status and one for workflow token to be updated in the DB.
These messages may be out of sync causing the dependent pipeline to fail with a NPE because of missing workflow token.
This change adds a validation that constraint should only be satisfied when a workflow token is written to the DB.
Assumptions
This is based on the assumption that all pipelines add save a workflow token whenever it terminates(in both success and failure cases)
Testing
Actual state is very hard to reproduce so tried running dependent pipelines in local sandbox with scheduling pipeline1 and triggering pipeline2 whenever the first pipeline completes.