IWF-232: Add disabling sticky cache option #473
Conversation
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-232
branch
3 times, most recently
from
6ad8c6b
to
1f655b8
Compare
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-232
branch
from
1f655b8
to
d019aa3
Compare
| @@ -19,6 +19,10 @@ type InterpreterWorker struct { | |||
| tasklist string | |||
| } | |||
|
|
|||
| type StartOptions struct { | |||
| DisableStickyCache bool | |||
There was a problem hiding this comment.
I know we have a specific use case for this start option to disable sticky cache, but I'm wondering if we should add a warning comment about the performance impacts of doing this for other reasons. (My understanding is that this would have a not insignificant impact on performance in a non-test/real-world use case. Is that right?)
There was a problem hiding this comment.
That's right. I'll add a comment pointing it out
| @@ -37,12 +41,17 @@ func (iw *InterpreterWorker) Close() { | |||
| iw.worker.Stop() | |||
| } | |||
|
|
|||
| func (iw *InterpreterWorker) Start() { | |||
| func (iw *InterpreterWorker) Start(startOptions StartOptions) { | |||
There was a problem hiding this comment.
Could we keep the original Start function here and have a second StartWithOptions so that the launchTemporalService in iwf.go does not need to change? The Start function could just set the default StartOptions and call StartWithOptions - I think that's cleaner overall but in particular keeps launchTemporalService clean
Unless we think we have a use case where launchTemporalService would actually be calling start with different options based on runtime configuration values??
There was a problem hiding this comment.
That makes sense to me. Definitely, we can keep Start as it was and add StartWithOptions. I like the idea, thanks for this suggestion!
| iw.StartWithOptions(options) | ||
| } | ||
|
|
||
| func (iw *InterpreterWorker) StartWithOptions(startOptions StartOptions) { |
There was a problem hiding this comment.
To even better address @mixydubs concerns, I feel like it's even better to have this StartWithStickyCacheDisabledForTest() so that no one would use this in production.
StartWithOptions is also a little confusing with the workerOptions that we are passing through config config.Config in NewInterpreterWorker. If we really need to pass options, they can be passed from there for consistency.
| @@ -13,12 +13,19 @@ jobs: | |||
| # Give the default GITHUB_TOKEN write permission to commit and push the | |||
| # added or changed files to the repository. | |||
| contents: write | |||
| strategy: | |||
There was a problem hiding this comment.
I thought we wanted have one action to run with stickyCache, and another run without, right? (two actions run in parallel so that it won't slow down)
So I expect we need new yml files like:
- ci-temporal-integ-test-disable-sticky.yml
- ci-cadence-integ-test-disable-sticky.yml
There was a problem hiding this comment.
Got it. I thought we would only do tests with no sticky cache, but it makes sense to keep both scenarios 👍
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-232
branch
from
3431002
to
fe750a4
Compare
| $Q go test -v ./integ -search=false -temporal=false -dependencyWaitSeconds=180 -disableStickyCache=true | ||
|
|
||
| ci-cadence-integ-test-disable-sticky: | ||
| $Q go test -v ./integ -run "(?i)^Test[${startsWith}]" -search=false -temporal=false -dependencyWaitSeconds=180 -disableStickyCache=true | ||
|
|
||
| ci-temporal-integ-test: |
There was a problem hiding this comment.
Can you test does -disableStickyCache=true work? I thought it has to be -disableStickyCache because it's a bool flag
There was a problem hiding this comment.
It does work, but I can simplify it to disableStickyCache
Description
Checklist
Related Issue
Closes #466