Add --allow-no-pollers option for SetCurrentVersion and SetRampingVersion
#8254
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
carlydf
force-pushed
the
allow-no-pollers
branch
from
687eeb2 to
c34bf2e
Compare
tests/worker_deployment_test.go
Outdated
| } | ||
|
|
||
| func (s *WorkerDeploymentSuite) TestSetWorkerDeploymentCurrentVersion_NoPollers() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), time.Second*60) // verify task queue versioning info is taking a lonng time :/ |
There was a problem hiding this comment.
I think it's because the user data propagation step occurs after the worker polls (second to last line of the test), which means the last two lines of the test take a while to complete. It could be interesting to use a separate context for the last operation so that we are more specific about which parts are taking which amounts of time ... 60s is a lot and does make me a bit uncomfortable. I'll try that out
| s.ensureCreateDeployment(tv) | ||
| } else { | ||
| s.ensureCreateVersionInDeployment(tv) | ||
| if !allowNoPollers && ensureSystemWorkflowsExist { |
There was a problem hiding this comment.
not sure i see the reason why we should have ensureSystemWorkflowsExist (I might be missing some context) - can we not just simplify things by only checking !allowNoPollers? In other words, don't we always have to ensure systemWorkflows exist when allowNoPollers == false?
There was a problem hiding this comment.
For some of the allow-no-pollers tests I want there to be no system workflows and also !allow-no-pollers so that I can check the error message is what we expect
| s.verifyTaskQueueVersioningInfo(ctx, tv.TaskQueue(), tv.DeploymentVersionString(), "", 0) | ||
| } | ||
|
|
||
| func (s *WorkerDeploymentSuite) TestSetWorkerDeploymentRampingVersion_NoPollers() { |
There was a problem hiding this comment.
I wonder: Do we wanna have a slightly complicated end-to-end test to ensure that this newly added feature works as intended?
I think these tests should be there, but what I was thinking is just having a test that builds on top of this (also ok to just build on top of this)
- do a describe on the worker deployment workflow: This could be important because we now have added code to separately add a version to the local state of the deployment and it would be nice to verify if we do indeed see this version in the output.
- delete this version and we could then verify if the version has been removed from the local state? Same reason as above since we are playing with the local state of the deployment workflow from within these API calls.
There was a problem hiding this comment.
I added a describe call! I don't think we need to do a delete version -- the describe call confirms that the workflow state is the same as if the version was created by a poller
| a.Equal(expectedCurrentVersion, tqDesc.GetVersioningInfo().GetCurrentVersion()) //nolint:staticcheck // SA1019: old worker versioning | ||
| a.Equal(expectedRampingVersion, tqDesc.GetVersioningInfo().GetRampingVersion()) //nolint:staticcheck // SA1019: old worker versioning | ||
| a.Equal(expectedPercentage, tqDesc.GetVersioningInfo().GetRampingVersionPercentage()) |
There was a problem hiding this comment.
confused: wouldn't this function without these changes work over here? I had a peek at some of the matching code and don't seem to see changes directly related to the API
There was a problem hiding this comment.
I moved the a.Nil(err) check to after the expected versions check because when it was being checked before, the test would eventually fail with "context timed out" error, when really the reason it timed out was due to one of the a.Equal checks failing. It was easier to debug the failing tests when I could see what the actual mismatch was, instead of just "context deadline exceeded".
You're right, I don't think the other changes are needed though. I made those changes because when a task queue is totally unversioned (nil ramp and nil current version), the VersioningInfo is nil, which makes the expected ramping and current versions "" instead of __unversioned__. As a test writer, I felt like it was easier to reason about the test if I put "expected unversioned" instead of "expect empty string" .. but tbh now that I think about it more, I think that's the wrong approach. Will change!
There was a problem hiding this comment.
reverted and all still works!
What changed?
Add
--allow-no-pollersoption for SetCurrentVersion and SetRampingVersionWhy?
Users want to be able to opt-in to setting versions that have not yet had any pollers to current or ramping
How did you test it?