-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add workflow invocation grabbing with db-skipped-lock #10177
Conversation
e142050
to
7f95da9
Compare
I had grabbable workflow scheduler assignment working back when I added it for jobs and was told by @jmchilton not to enable it because then workflow invocations would interleave outputs when run in a single history. It was my recollection that you could enable it by explicit configuration in the |
I don't think it was finally implemented, that's what #8209 is about.
That said, I think the |
There is a bunch of effort to override db-skip-locked in a variety of handler assignment scenarios unless the admin explicitly sets it in the workflow schedulers config, it looks like you didn't touch that (although changing incompatible methods might have an effect on that)? |
Yeah, I haven't checked if just setting |
I guess that override should probably follow the value of |
dc8b83e
to
83bced9
Compare
83bced9
to
78daef3
Compare
Works fine, I've added a test case for this.
Not sure I understand this. Do you agree with me that But all these concerns also apply to standalone workflow schedulers, so maybe that can be a followup ? |
👍 I think this is all ok and that in-history serialization is being addressed via other means? |
That's my thinking! |
Can this be backported to 20.05? |
@innovate-invent I'll leave that up to Marius, going to go ahead and get this into the dev branch though. |
I don't think we'd want to make these large-ish changes with a couple of different consequences (see the discussion about serial workflow scheduling above) to an existing release. We're hoping to get 20.09 out in 2 weeks though, so this shouldn't be far away from appearing in a stable release. |
2 weeks sounds great! Thanks! |
This PR does not seem to work. Invocations are not being grabbed.
Tried adding |
Did this work for you in the end @innovate-invent ? I think I use it in a similar way than you do (not specifying handlers in the job conf and just joining them to the pool). I currently have to use a trick with gxadmin to assign those workflows to handlers, but I was hoping to get out of that trick. |
My job handlers run with --attach-to-pool=job-handlers and I just use the default workflow scheduler configs otherwise. The job handlers are configured to use db-skip-locked. There was an issue with separating the job handlers and workflow invocation handlers related to the maximum_workflow_jobs_per_scheduling_iteration config. I don't know if that was ever resolved. Edit: Going back through the issues, it looks like I got this to work with #10371 but never left it enabled for some reason. |
and db-transaction-isolation.
Closes #8209.
Needs some tests and the grabbing logic should be its own class that can be shared with the job grabber.