Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(aci): enqueue workflows for delayed processing #83548
feat(aci): enqueue workflows for delayed processing #83548
Changes from 5 commits
c3ec931
9f8ea09
8630b00
30739b2
0ffd148
6a51006
be626eb
4b1708d
44035ae
6bbdb9a
84ee9ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
can we audit the logging in here? I'm not sure we need a lot of these info logs anymore (this one for example is being sampled to 1% - which isn't super valuable)
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.
@mifu67 are the logs you've downsampled to 1% for enqueue rules for delayed processing still useful?
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.
she just did that cause they were noisy, i think there are a number of info logs here that we need to audit.
I'd recommend only keeping things that make sense to you. if you have any questions lemme know :)
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.
this reads a little strange - why is the var name
if_dcgs
? could it just bedcgs
?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.
these are IF data condition groups
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.
🤔 maybe we call them
workflow_action_filters
? (since that should be the type of DCG 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.
i don't think we should be doing filtering or anything here - this method should be a pure method to evaluate the workflow triggers and that's it; if we want to filter the workflows being evaluated we should do that before evaluating them.
let's update the code to have
process_workflows
figure out what is fast / slow conditions, then filter based on fast / slow conditions.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 only enqueue workflows that need to have slow conditions evaluated because they don't pass after evaluating the fast conditions alone. are you saying to evaluate the workflows with slow conditions separately? some of them might be triggered immediately and some of them may have to be enqueued
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 to only return
triggered_workflows