Fix: change default HTTP timeout to 60s#1301
Conversation
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
|
||
| func NewApiClient(cfg ClientConfig) *ApiClient { | ||
| // Set defaults for config values that are not set. | ||
| cfg.HTTPTimeout = time.Duration(orDefault(int(cfg.HTTPTimeout), int(30*time.Second))) |
There was a problem hiding this comment.
I'd like the ApiClient to "own" its default values — see tests were the client is hard coding a value that is fundamentally controlled by the caller. Could we do the opposite by (i) removing the upstream default, and (ii) setting the value to 60 seconds here?
There was a problem hiding this comment.
The API client already has the timeout set. This PR removes a duplicate line of code.
There was a problem hiding this comment.
Oh I see, I misunderstood the change as being about deferring the default logic to the Databricks config. All good 👍
|
|
||
| func NewApiClient(cfg ClientConfig) *ApiClient { | ||
| // Set defaults for config values that are not set. | ||
| cfg.HTTPTimeout = time.Duration(orDefault(int(cfg.HTTPTimeout), int(30*time.Second))) |
There was a problem hiding this comment.
Oh I see, I misunderstood the change as being about deferring the default logic to the Databricks config. All good 👍
2 participants
What changes are proposed in this pull request?
This PR fixes the default configuration for HTTP timeout. A couple of months ago we increased the timeout to 60s, but that was a no-op because of this line that sets it to 30 seconds.
I caught it after noticing a couple of
request timed out after 30s of inactivityduring testing.How is this tested?
Existing unit test.
NO_CHANGELOG=true