Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(query): make config validation for query controller less strict #21324

Merged
merged 4 commits into from
Apr 28, 2021

Conversation

danxmoran
Copy link
Contributor

Related to #21321

In 2.0.4:

  • query-concurrency: 0 was not allowed
  • The default values of query-concurrency and query-queue-size were both 10

In practice, we found that users quickly hit the default threshold. See #17912.

In 2.0.5:

  • query-concurrency: 0 was enabled as a short-hand for infinite concurrency
  • Validation was added to assert that:
    • query-queue-size is 0 when query-concurrency is 0
    • query-queue-size is > 0 when query-concurrency is > 0
  • The default values of query-concurrency and query-queue-size were both updated to be 0

This is guaranteed to be a breaking change for any users that had updated their deployments to override the config value for query-concurrency without also updating query-queue-size, because concurrency must have been > 0 and queue-size would now be 0 by default.

This PR:

  • Changes the default values for query-concurrency and query-queue-size to 1024
  • Loosens our config validation to allow query-queue-size > 0 when query-concurrency is 0

The new non-zero defaults should prevent the guaranteed breaking change from 2.0.5, but still give more breathing room to users that have been fighting the concurrency limits in older versions. The validation change should make it smoother for users who want to make use of the new infinite-concurrency feature.

@davidby-influx davidby-influx self-requested a review April 28, 2021 20:37
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Any desire for a maximum Queue size?

@danxmoran
Copy link
Contributor Author

Any desire for a maximum Queue size?

None that I've seen 🙂 most users (understandably) seem to wish it was unlimited, but that doesn't work with our use of a chan for the queue. I experimented with setting maxint as the queue size and was surprised to learn that make(chan, N) allocates enough space for the buffer of N up-front, so server startup either takes forever or OOMs when the number is "too big".

@danxmoran danxmoran merged commit 942f709 into master Apr 28, 2021
@danxmoran danxmoran deleted the dm-flux-controller-config-validate-less-strict branch April 28, 2021 21:27
danxmoran added a commit that referenced this pull request Apr 28, 2021
…21324)

* fix(query): accept queue-size > 0 when concurrency = 0
* fix(influxd): revert defaults for query settings to avoid validation err
* test: lower the default query concurrency used by test launchers
Copy link
Contributor

@davidby-influx davidby-influx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, lower values only for testing, not for user defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants