Run event loop after job application #717
Conversation
Sushisource
force-pushed
the
run-loop-after-jobs
branch
from
71b3ca3 to
3d74ddf
Compare
Sushisource
force-pushed
the
run-loop-after-jobs
branch
from
3d74ddf to
a901b4d
Compare
| # Ensure the main workflow function task is initialized after a first run of the | ||
| # event loop, which might execute before-start signals/updates. This is behind | ||
| # a finally because if those handlers fail, we need still need the main routine | ||
| # to be initialized in order to fail tasks properly. |
There was a problem hiding this comment.
If it weren't for this bit we would've ended up with net-less code here. A bit too bad, but, I think still nicer than grouping things into batches.
Sushisource
force-pushed
the
run-loop-after-jobs
branch
from
c292f59 to
53462cc
Compare
Sushisource
force-pushed
the
run-loop-after-jobs
branch
from
53462cc to
cac670c
Compare
| self.events.append("act-2") | ||
|
|
||
|
|
||
| async def test_async_loop_ordering(client: Client): |
There was a problem hiding this comment.
Hrmm, I think another test may look like this:
- Create workflow that has a signal handler that updates some counter and an activity in the background that, when done, updates that same counter. And have the primary workflow method return the counter when it is
> 2(i.e. after wait condition for that). - Start the workflow, and while the activity is running, kill the workflow worker
- While the workflow worker is not running, send two signals and complete the activity
- Start the workflow worker again and confirm result and confirm that all three of those items (the two signals and the activity completion) came in the same workflow task/activation.
I think with older code, the counter will only reach 2 before workflow returns (i.e. the two signals), but this newer code may return 3 (because it completed the activity before it processed wait condition). I have not tested this though.
There was a problem hiding this comment.
(I assume you meant >=2, otherwise it's always the same). Bleh, yes indeed that does cause a difference, makes sense.
Well, then we've gotta decide if this is worth a flag or not too. Somewhat more worth it maybe since the batches are kind of odd, but, if we're going to have to keep that code around forever anyway... 🤷♂️. What do you think?
At minimum these tests are kind of useful to keep around.
There was a problem hiding this comment.
To be honest, I think the benefit of code/logic cleanup goes away if you have to keep the old stuff around. But I agree the test is very valuable even as a separate PR.
There was a problem hiding this comment.
This is where it's relevant that we have other changes that we'd like to make that will also require a flag I think?
I think the benefit of code/logic cleanup goes away if you have to keep the old stuff around.
I don't think I agree with that. For readers of the code it's an improvement to be given the "nicest" implementation to read, and also be referred to the location of a previous implementation that can be hidden away somewhat.
There was a problem hiding this comment.
Readers of code will have two implementations to read which is worse, and I still think it's not worth it to help readers of code if there are other issues (such as maintaining two forked pieces of logic). Having said that, I agree if we solved the wait condition issue at the same time this could be worth it. That has actual tangible user benefit, not just code reader benefit.
There was a problem hiding this comment.
Also, if you read what I wrote you'll see that my point is that readers don't need to read the old implementation. They can read the new one, and unless they have reason to be interested in it, take it on trust that the old one is equivalent.
There was a problem hiding this comment.
What I'm saying is that I don't think the cost of having two implementations around is worth the benefit of simplifying one of them for internal code readers. I also don't think we should have runtime side effects on users, e.g. SDK flags in history, if the only benefit is internal code legibility. Maybe if there were other benefits, sure. But maybe we can combine with actual user benefit like the wait condition thing.
There was a problem hiding this comment.
What I'm saying is that I don't think the cost of having two implementations around is worth the benefit of simplifying one of them.
Yes, you already gave that opinion, and then I gave a countering opinion. After that point neither of us have added anything to the conversation; in that situation there's no need for us to compose further replies.
I also don't think we should have runtime side effects on users, e.g. SDK flags in history, if the only benefit is internal code legibility. But maybe we can combine with actual user benefit.
Yes, I already said that:
This is where it's relevant that we have other changes that we'd like to make that will also require a flag I think?
There was a problem hiding this comment.
I agree with Chad I think. Having two is worse than one nicer and one old, I think. If we had a policy around being able to rip out the old one at some point, that would change my mind.
If you want to look at the "nice" implementation, you can look at Ruby now as a reference so that's somewhat redeeming.
I can split out the tests to another PR.
There was a problem hiding this comment.
Sounds good. I'm definitely going to keep in mind that we could bundle this with improving wait_condition.
|
Closing, tests ported to #729 |
What was changed
Run the event loop after applying all jobs, rather than after each "batch" of jobs.
Why?
Align with TS & Core's intended method of job application
Checklist
Closes [Feature Request] Consider aligning activation job application with TS changes #606
How was this tested:
Existing & new tests
Any docs updates needed?