expose DisableStaggerStart via TestOnlyConfig - #385
Closed
bgentry wants to merge 1 commit into
Closed
Conversation
Contributor
|
As discussed on Slack, my fear with the granularity here is that it's not very forward compatibility friendly. Meaning that in the future if more options were to be added like I was thinking about this one a little more overnight — what do you think about exposing |
brandur
added a commit
that referenced
this pull request
Jul 3, 2024
… start This one's a continuation of #385. Maintenance services have a staggered start feature that causes them to sleep for a random amount of jittered time on startup so they don't all try to work simultaneously. This is useful in production, but somewhat harmful in tests because it makes start and stop slower and thereby integration test cases slower. River has an internal flag that allows staggered start to be disabled in its own test suite, but external users of River have no way to access this functionality. Here, introduce `Config.TestOnly` that can be provided to client configuration in test suites regardless of whether the caller is internal or not. This differs slightly from #385 in that it provides only a boolean, with the idea being that if we find it useful to disable other features for tests in the future, a boolean keeps third party code for forwards compatible in that they get these disabled automatically. In case it does become important to distinguish between individual features at some later time, I figure we can add an additional `TestOnlyConfig` property that allows full configuration beyond defaults.
brandur
added a commit
that referenced
this pull request
Jul 3, 2024
… start This one's a continuation of #385. Maintenance services have a staggered start feature that causes them to sleep for a random amount of jittered time on startup so they don't all try to work simultaneously. This is useful in production, but somewhat harmful in tests because it makes start and stop slower and thereby integration test cases slower. River has an internal flag that allows staggered start to be disabled in its own test suite, but external users of River have no way to access this functionality. Here, introduce `Config.TestOnly` that can be provided to client configuration in test suites regardless of whether the caller is internal or not. This differs slightly from #385 in that it provides only a boolean, with the idea being that if we find it useful to disable other features for tests in the future, a boolean keeps third party code for forwards compatible in that they get these disabled automatically. In case it does become important to distinguish between individual features at some later time, I figure we can add an additional `TestOnlyConfig` property that allows full configuration beyond defaults.
Contributor
Author
|
Superseded by #414. |
brandur
added a commit
that referenced
this pull request
Jul 3, 2024
… start (#414) This one's a continuation of #385. Maintenance services have a staggered start feature that causes them to sleep for a random amount of jittered time on startup so they don't all try to work simultaneously. This is useful in production, but somewhat harmful in tests because it makes start and stop slower and thereby integration test cases slower. River has an internal flag that allows staggered start to be disabled in its own test suite, but external users of River have no way to access this functionality. Here, introduce `Config.TestOnly` that can be provided to client configuration in test suites regardless of whether the caller is internal or not. This differs slightly from #385 in that it provides only a boolean, with the idea being that if we find it useful to disable other features for tests in the future, a boolean keeps third party code for forwards compatible in that they get these disabled automatically. In case it does become important to distinguish between individual features at some later time, I figure we can add an additional `TestOnlyConfig` property that allows full configuration beyond defaults. --------- Co-authored-by: Blake Gentry <blakesgentry@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I ran into an issue with some external testing where I needed to run a full River client. These tests were quite slow, and I realized it was because of
StaggerStartStop—a setting that is currently not exposed outside theriverpackage.I'm not sure this is the right way to expose it. If we end up having a set of related knobs that can be tweaked for test usage (lowering buffers, sleep durations, etc) it's likely most of the time they'll be used all together. Open to other ways to expose this.