-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add BlockQueryWaitTime
Nomad Configuration Option
#755
Conversation
de94069
to
0ebd040
Compare
|
||
// BlockQueryWaitTime controls how long Nomad API requests supporting blocking queries | ||
// are held open. Defaults to 5m. | ||
BlockQueryWaitTime time.Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note regarding the option name choice: I waffled between a few options before settling on this. Primarily motivated by this bit of similar config in the Nomad codebase (though related to a Consul API client setting in that case 🤷🏻): https://github.com/hashicorp/nomad/blob/v1.7.0-beta.1/client/config/config.go#L380-L387
(Happy to call it whatever though 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name looks good to me 👍
WaitTime
General Configuration OptionBlockQueryWaitTime
Nomad Configuration Option
BlockQueryWaitTime
Nomad Configuration OptionBlockQueryWaitTime
Nomad Configuration Option
@jrasell: Would it be possible to get this PR reviewed ahead of the next autoscaler release? 🙏🏻 |
This value is now set on the Nomad api.Client config directly. :toot:
0ebd040
to
65e0f45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @jeffwecan!
And apologies for the delay in getting this merged. Nomad 1.7 and Autoscaler HA kept us busy for a while. I'm going over the opened PRs and issues to prepare a new release.
|
||
// BlockQueryWaitTime controls how long Nomad API requests supporting blocking queries | ||
// are held open. Defaults to 5m. | ||
BlockQueryWaitTime time.Duration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name looks good to me 👍
This PR adds a
BlockQueryWaitTime
config file option /-nomad-block-query-wait-time
CLI flag to be used when folks want to either:I.e., Resolves #754
Aside from the test case updates included as part of this PR's changes, a bit of practical testing was performed as well. Specifically with a shorter-than-5m-default wait time, but still longer than our 60 LB timeout, configured for the autoscaler:
Combined with our dev deployment's 60 second idle timeout Nomad LB, we hit the precipitating error but now with our non-default
2m
/120000ms
wait time reflected (yay):Subsequently, we set
block_query_wait_time = "56s"
which was seen to prevent such errors with our particular Nomad API connectivity arrangements.Also going to link the initial PR adding blocking queries here (for general context + so there is a breadcrumb to this PR in that preceding one): #38