✨ Add optional description parameter to #run and #in_parallel#44
✨ Add optional description parameter to #run and #in_parallel#44danielvdao wants to merge 2 commits into
description parameter to #run and #in_parallel#44Conversation
|
Hello @TheMetalCode and @lucaslazzaris! I noticed you two are owners and recently contributed. Is there any way I can have eyes on this PR? Thank you. |
|
@danielvdao Hey, sorry for the delay; I'll try to get some 👀 on this (my own or others). Thanks for your patience! |
|
@austenmadden much appreciated and looking forward to hearing back! |
|
Hi @austenmadden, any updates? Wanted to check in here. |
austenmadden
left a comment
austenmadden
left a comment
There was a problem hiding this comment.
@danielvdao really appreciate taking the time to invest in the gem and the idea is great!
I've had the same problems with understanding how the batch correlates to a workflow step. This is definitely a rough spot in the gem. That said, I think changing the internal data structure this dramatically for this feature is an issue.
Couple paths forward:
- Finding a way to implement this feature that doesn't result in a breaking change to the internal data structure of
Orchestration. - Perhaps reflecting on the jobs/step definition to create a more meaningful auto generated batch name than just the
stepindex?
|
|
||
| ## [1.1.0] | ||
| * Add a new parameter for descriptions to `#run` and `#in_parallel` | ||
| [#43](https://github.com/doximity/simplekiq/pull/44) |
| @parallel_workflow = [] | ||
| yield | ||
| serial_workflow << @parallel_workflow if @parallel_workflow.any? | ||
| serial_workflow << {parallel: @parallel_workflow, description: description} if @parallel_workflow.any? |
There was a problem hiding this comment.
As I understand it this change would break existing workflows in flight? That'd be a fairly major issue for us internally. Changing the internal data structure like this IMO would require a major version update... which IMO isn't ideal for a feature of this nature.
So I suppose my challenge would be can you implement this feature without breaking the internal structure we have here?
| step.map do |(job, *args)| | ||
| @serialized_workflow ||= serial_workflow.map do |item| | ||
| result = if item[:parallel] | ||
| jobs = item[:parallel].map do |entry| |
There was a problem hiding this comment.
If I'm reading this correctly we ignore descriptions for run calls inside a parallel group?
There was a problem hiding this comment.
context "when run steps inside in_parallel have descriptions" do
before do
orchestration.in_parallel do
orchestration.run OrcTest::JobA, "syn", description: "Job A description"
orchestration.run OrcTest::JobB, "ack", description: "Job B description"
end
end
it "includes the descriptions in the serialized jobs" do
expect(subject).to eq [
{
"jobs" => [
{"description" => "Job A description", "klass" => "OrcTest::JobA", "args" => ["syn"]},
{"description" => "Job B description", "klass" => "OrcTest::JobB", "args" => ["ack"]}
]
}
]
end
endHere's an example test that demonstrates what I'm getting at. Currently fails on this branch.
| end | ||
|
|
||
| def run(*step) | ||
| def run(*step, description: nil) |
There was a problem hiding this comment.
It's sorta obscure but there could be a collision here if someones job happened to accept a description as a named argument. Somewhat bad practice to design jobs like that in Sidekiq IMO but it does happen.
A solution to that would be a more explicit argument name like step_description or simplekiq_step_description. Not perfect but dramatically reduces the chance vs just description.
There was a problem hiding this comment.
Sounds good, I can change it 👍🏽
|
@austenmadden thanks for taking a look, I appreciate it. I agree that this would break in-flight jobs, that's a miss on my part. Let me take a stab at reflecting on this one more. Would appreciate any suggestions you have as well in the meantime. |
2 participants
When adding jobs to our orchestration, we've had difficulty understanding which part of the flow we're on. Our workflow is long and we thought it would be nice to be able to add more descriptions to the steps.
This adds an optional description parameter and also adds a few tests alongside that. Would love any opinions and appreciate the contribution to the community for this gem.