make status checks configurable from env vars + use shorter replication interval for testing #10368
Conversation
github-actions
bot
added
area/platform
|
Haha so we had some discussion around whether or not these status check intervals should be configurable via env-vars, thread here: https://docs.google.com/document/d/1w9UBa1xTQcpJYLjmWi8zMLYN3vZ6DhDVyTxFIM5hgxk/edit?disco=AAAAVLvHHqI We didn't consider acceptance testing as a use case for wanting to configure shorter intervals, so I think this makes sense. @cgardens @davinchia FYI in case you have a stronger opinion than I do |
There was a problem hiding this comment.
See my comment about the hardcoding discussion, I don't think the opinions expressed were strong so I'm comfortable moving forward with this to speed up our tests. I kind of doubt we'll see much/any usage from the configuration of non-replication-worker intervals but it doesn't hurt for the sake of consistency
| * | ||
| * In seconds if specified by environment variable. | ||
| */ | ||
| Duration getDefaultWorkerStatusCheckInterval(); |
There was a problem hiding this comment.
can you go through the class doc string and follow that accordingly?
I'm guessing these should be marked as internal-use only since we don't want to expose them to OSS users.
There was a problem hiding this comment.
oops missed this.
Added in #10390
|
Looks like this shaves off almost 5 min! |
We spend quite a lot of time in Kubernetes integration testing just waiting for status checks. Here I'm allowing the values to be configurable in the environment and then using those for both of the Kuberentes acceptance tests, hopefully increasing the speed of the tests.
I split this into two commits so it'd be easier to review but also so we can get a sense of the speed up. I think this will help on local and CI iteration when running the acceptance tests. (EDIT: I forgot to push them individually, so I added two reverts so we can actually compare)
@pmossman I don't really have a use case for configuring the other values outside of replication, but I just figured it would make sense to have these easily configurable.