-
Notifications
You must be signed in to change notification settings - Fork 99
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
Fixes #25736 - Pass id of request which triggered a task into the task #383
Conversation
a17240b
to
07d99f7
Compare
07d99f7
to
3280cf1
Compare
Tested and works well, except covering exceptions logging, which could be solved by this adamruzicka#2 |
Dynflow 1.2.1 has been released, let's bump the version dependency |
app/lib/actions/recurring_action.rb
Outdated
@@ -10,11 +10,15 @@ def self.included(base) | |||
# Hook to be called when a repetition needs to be triggered. This either happens when the plan goes into planned state | |||
# or when it fails. | |||
def trigger_repeat(execution_plan) | |||
request_id = ::Logging.mdc['request'] | |||
::Logging.mdc['request'] = nil |
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.
After discussion with @ponrjek, we think the best way to support the recurring jobs would be to assign it a new (but non-empty) request id, so that we can group the related tasks from every iteration together (otherwise, we can do it just with the first iteration)
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.
Should I also log that?
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 canl handle the logging part in separate PR
Two last comments before merging this. |
And rely on Dynlfow's `run_user_code` to clean it once not needed.
1948f6d
to
47f6094
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.
Pending fixing tests. Otherwise works well and can be merged once CI is green
# Perform the planning (trigger repeat) | ||
task.execution_plan.delay_record.plan | ||
repetition = recurring_logic.tasks.find { |t| t.id != task.id } | ||
repetition.input[:current_request_id].must_be :nil? |
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 needs to be updated as well
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.
Everything's good now
Merged as 12b53f2, thanks @adamruzicka |
No description provided.