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

[CDAP-21064] Validate workflow token on program status trigger. #15810

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ritwiksahani
Copy link
Contributor

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.

@ritwiksahani ritwiksahani added the build Triggers github actions build label Jan 17, 2025
Copy link
Contributor

@tivv tivv left a 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));
Copy link
Contributor

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 {
Copy link
Contributor

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>() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it a lambda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Triggers github actions build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants