fix(upgrade): ensure query config written by influxd upgrade is valid
#21321
Conversation
This file contains 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
danxmoran
commented
Apr 28, 2021
| case int32: | ||
| concurrency = int64(c) | ||
| case int64: | ||
| concurrency = c |
There was a problem hiding this comment.
All these cases are paranoia on my part; I'm not sure when the config-loader would pick each type of int.
danxmoran
commented
Apr 28, 2021
| } | ||
|
|
||
| // When query-concurrency is > 0, query-queue-size must also be > 0. | ||
| v2Config["query-queue-size"] = concurrency |
There was a problem hiding this comment.
There's a risk here that:
- Somebody will pick a huge value for query-concurrency
- We write the same value into query-queue-size
- At startup, the server uses the huge value for query-queue-size as the size of a new
chan, consuming a lot of memory all at once
I'm assuming the risk is low now that we support 0 for unlimited, so most people who would be writing huge concurrency values can now write 0 instead.
davidby-influx
approved these changes
Apr 28, 2021
danxmoran
added a commit
that referenced
this pull request
Apr 28, 2021
…id (#21321) * test: refactor upgrade test to cover the config upgrade * fix: ensure upgraded query config is valid
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #21315
0to10when upgradingquery-concurrencyconfig, now that 0 is a valid valuequery-concurrency, ensure we also write a > 0 value forquery-queue-sizeI had to refactor a bunch of things to make our upgrade-real-DB regression test actually test loading upgraded config. I confirmed that the 1st commit fails with the error seen in #21315, and the 2nd commit gets things to pass again.