Do not close all TCP connections on an HTTP error #1179

shreyas-goenka · 2025-03-18T17:00:44Z

What changes are proposed in this pull request?

Closing the TCP pool on every error is unnecessary. The function was meant to be used for http clients that are short lived to avoid leaking resources (ref: golang/go#26563). For the Go SDK, the HTTP client is long-lived, and thus, there's no reason to proactively close the TCP connection pool on errors.

By default, the HTTP protocol maintains the health of its TCP connections (keep-alive messages) and the Go HTTP client automatically closes connections that have been idle for too long based on a timeout.

How is this tested?

Manually used this in a Databricks CLI binary and verified that the commands still work.

NO_CHANGELOG=true

### Internal Changes * Bump staticcheck to 0.5.1 and add go 1.23 test coverage ([#1106](#1106)). * Bump x/net, x/crypto dependencies ([#1107](#1107)). * Create custom codeql.yml ([#1114](#1114)). * Decouple serving and oauth2 package ([#1110](#1110)). * Migrate workflows that need write access to use hosted runners ([#1112](#1112)). * Move package credentials in config ([#1115](#1115)). * Update Queries test ([#1104](#1104)). ### API Changes: * Added `NoCompute` field for [apps.CreateAppRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/apps#CreateAppRequest). * Added `HasMore` field for [jobs.BaseJob](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#BaseJob). * Added `HasMore` field for [jobs.BaseRun](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#BaseRun). * Added `PageToken` field for [jobs.GetJobRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#GetJobRequest). * Added `HasMore` and `NextPageToken` fields for [jobs.Job](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#Job). * Added `HasMore` field for [jobs.Run](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#Run). * Added `RunAs` field for [pipelines.CreatePipeline](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/pipelines#CreatePipeline). * Added `RunAs` field for [pipelines.EditPipeline](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/pipelines#EditPipeline). * Added `AuthorizationDetails` and `EndpointUrl` fields for [serving.DataPlaneInfo](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/serving#DataPlaneInfo). * [Breaking] Changed `Update` method for [a.AccountFederationPolicy](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/oauth2#AccountFederationPolicyAPI) account-level service with new required argument order. * [Breaking] Changed `Update` method for [a.ServicePrincipalFederationPolicy](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/oauth2#ServicePrincipalFederationPolicyAPI) account-level service with new required argument order. * Changed `UpdateMask` field for [oauth2.UpdateAccountFederationPolicyRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/oauth2#UpdateAccountFederationPolicyRequest) to no longer be required. * Changed `UpdateMask` field for [oauth2.UpdateServicePrincipalFederationPolicyRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/oauth2#UpdateServicePrincipalFederationPolicyRequest) to no longer be required. * [Breaking] Changed `DaysOfWeek` field for [pipelines.RestartWindow](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/pipelines#RestartWindow) to type [pipelines.DayOfWeekList](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/pipelines#DayOfWeekList). OpenAPI SHA: 779817ed8d63031f5ea761fbd25ee84f38feec0d, Date: 2025-01-08

…pace test (#1120) ## What changes are proposed in this pull request? What - The PR modifies the scope of directory traversal in the recursive list test. It limits the search to the ".sdk" directory, where the relevant notebook is created. Why - The test was previously hitting API rate limits because it was scanning too many elements in the current traversing directory. By narrowing the scope to the ".sdk" directory, the test can now run more efficiently without encountering these limits. ## How is this tested? This is itself an integration test.

## What changes are proposed in this pull request? This PR deletes the `examples/mocking` directory as it is no longer maintained. This mocker example is built on the assumption that every `service` has `WithImpl()` that is used to set the stubs for every `service`. But this is no longer the case. This mocker is not even able to compile. ## How is this tested? N/A

## What changes are proposed in this pull request? Support Query parameters for all HTTP operations. Previously, only GET/HEAD/DELETE were supported. ## How is this tested? * Added Unit Tests * Run Integration Tests

## What changes are proposed in this pull request? This PR introduces a download target which is a wrapper around `go mod download`. The rationale behind introducing this target is that we can change the underlying implementation of this target for `dev/sdk-mod` branch in which there is no go.mod at the root repository so we need to either manually run download for all the go modules or introduce a make target like [this](https://github.com/databricks/databricks-sdk-go/pull/1123/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52). One could say that we can just add this target for SDK mod but we need to update integration test workflows accordingly. I think this is a better way than having an if/else statement in the workflow. ## How is this tested? Locally run `make download`

## What changes are proposed in this pull request? #1124 fixed the handling of APIs that use query parameters and a body simultaneously. After that change, query parameters are always sent, even if set to the zero value. This PR addresses this, using the same logic for query parameters as for fields in the body: if a field is set to its zero value, it will be included in the query parameter if it is present in ForceSendFields. ## How is this tested? This behavior is dependend upon in the Terraform provider. I'll be using it in databricks/terraform-provider-databricks#4430 and verifying that there is no behavior change in the generated request. When using this PR in Terraform, tests asserting that the path doesn't include query parameters pass.

### Bug Fixes * Do not send query parameters when set to zero value ([#1136](#1136)).

## What changes are proposed in this pull request? This PR aims at eliminating long-tail latency due to OAuth token refreshes in scenarios where a single client is responsible for a relatively high (e.g. > 1 QPS) continuous outbound traffic. The feature is disabled by default — which arguably makes this PR a functional no-op. Precisely, the PR introduces a new token cache which attempts to always keep its token fresh by asynchronously refreshing the token before it expires. We differentiate three token states: - `fresh`: The token is valid and is not close to its expiration. - `stale`: The token is valid but will expire soon. - `expired`: The token has expired and cannot be used. Each time a request tries to access the token, we do the following: - If the token is `fresh`, return the current token; - If the token is `stale`, trigger an asynchronous refresh and return the current token; - If the token is `expired`, make a blocking refresh call to update the token and return it. In particular, asynchronous refreshes use a lock to guarantee that there can only be one pending refresh at a given time. The performance of the algorithm depends on the length of the `stale` and `fresh` periods. On the first hand, the `stale` period must be long enough to prevent tokens from entering the expired state. On the other hand, a long `stale` period reduces the length of the `fresh` period, thus increasing the refresh frequency. Right now, the `stale` period is configured to 3 minutes by default (i.e. 5% of the expected token lifespan of 1 hour). This value might be changed in the future to guarantee that the default behavior achieves the best performance for the majority of users. **For reviewers:** - This PR only uses the new cache in control-plane auth flows; I plan to send a follow-up PR to enable asynchronous refresh in data-plane flows once this one has been merged. - Interface `oauth2.TokenSource` is likely not sufficient for us and we would need one with a `Token` method that takes a `context.Context` as parameter: `Token(context.Context) (Token, error)`. ## How is this tested? Complete test coverage with a focus on various concurrency scenarios.

…Context` (#1141) ## What changes are proposed in this pull request? This PR adds a new `TokenSource` interface which is essentially the same as `oauth2.TokenSource` but takes a `context.Context` as input. The rationale behind this interface is that many of the SDK `TokenSource` end-up making network calls under the hood. We want these to be done passing the context of the calling method. This interface is meant to ultimately entirely replace the use of `oauth2.TokenSource` in the SDK. ## How is this tested? Simple unit tests.

…1142) DO NOT MERGE: `api.go` and `impl.go` are here to help with review and will be removed before merge. These are supposed to be added later on via code generation. ## What changes are proposed in this pull request? This PR is a step towards enabling asynchronous refreshes of data plane tokens. The PR introduces a new interface `dataplane.EndpointTokenSource` which returns data plane tokens for a given endpoint and authorization detail. The default implementation relies on `auth.NewCachedTokenSource` to wrap the underlying TokenSources. Note: async refreshes are disabled at the moment and will be enabled in a follow-up PR. **About Config.GetTokenSource():** My first attempt to implement `GetTokenSource()` returned an error if the `TokenSource` could not be built (e.g. because of config initialization errors). This didn't work because: 1. service constructors do not return errors and thus had to panic; 2. service constructors are called systematically as soon as the `WorkspaceClient` is created — sometimes with a config that is not compatible with `GetTokenSource()`. Returning a `TokenSource` provider instead of a `TokenSource` would have solved the problem but did not feel right. The current solution (i.e. return an ever-failing token source) is the cleanest solution I could think off. ## How is this tested? Complete unit test coverage of the new package.

## What changes are proposed in this pull request? This PR enables async refreshes for cached token sources. This PR also: - Fixes a race condition bug in the unit tests by adding a small delay to the test refresh. - Remove one test in AzureCLI that is not correct anymore due to how the code is structured. Refactoring this to guarantee that the test pass is not trivial and ultimately out of the scope of the new system — as the cached token source is the one providing the guarantee. ## How is this tested? Fix unit tests.

### New Features and Improvements * Enable async refreshes for OAuth tokens ([#1143](#1143)). ### Internal Changes * Add support for asynchronous data plane token refreshes ([#1142](#1142)). * Introduce new TokenSource interface that takes a `context.Context` ([#1141](#1141)). ### API Changes: * Added `GetMessageQueryResultByAttachment` method for [w.Genie](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/dashboards#GenieAPI) workspace-level service. * Added `Id` field for [apps.App](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/apps#App). * Added `LimitConfig` field for [billing.UpdateBudgetPolicyRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/billing#UpdateBudgetPolicyRequest). * Added `Volumes` field for [compute.ClusterLogConf](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/compute#ClusterLogConf). * [Breaking] Removed `ReviewState`, `Reviews` and `RunnerCollaborators` fields for [cleanrooms.CleanRoomAssetNotebook](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/cleanrooms#CleanRoomAssetNotebook). OpenAPI SHA: 99f644e72261ef5ecf8d74db20f4b7a1e09723cc, Date: 2025-02-11

### Internal Changes * Do not send ForceSendFields as query parameters.

## What changes are proposed in this pull request? The existing code only paginates tasks and iterations. With this PR we update the logic to also paginate job_clusters, job_parameters and repair_history. This changes are needed for [Jobs API 2.2](https://docs.databricks.com/api/workspace/jobs/getrun) compatibility. ## How is this tested? I enabled API 2.2 calls by modifying URL string `/api/2.2/jobs/runs/get` in service/jobs/impl.go. Then I ran unit test from service/jobs/ext_api_test.go

## What changes are proposed in this pull request? Introduces logic in extension for jobs GetJob call that paginates tasks and other arrays in the response. This change is necessary for SDK and API 2.2 compatibility. API 2.2 serves paginated responses as long as next_page_token field is present in the response. The pagination logic is not exposed to the customer. ## How is this tested? I enabled API 2.2 calls by modifying URL string /api/2.2/jobs/get in service/jobs/impl.go. Then I ran unit test from service/jobs/ext_api_test.go

## What changes are proposed in this pull request? Enable Automated Tagging for SDK to support faster releases. ## How is this tested? Automated Tagging process has been already enabled in `terraform-provider-databricks` repository.

…#1150) ## What changes are proposed in this pull request? Introduces logic in the extension for jobs ListJobs call. The extended logic accounts for the new response format of API 2.2. API 2.1 format returns all tasks and job_cluster for each job in the jobs list. API 2.2 format truncates tasks and job_cluster to 100 elements. The extended ListJobs logic calls GetJob for each job in the list to populate the full list of tasks and job_clusters. I added logic that reads jobs from the list and produces custom iterator struct `expandedJobsIterator` that is supposed to mimic python generators. The goal is to only read necessary elements from the API endpoint and not more. ## How is this tested? Unit tests and manual tests. Manual tests were done in two modes: using API 2.2 and using API 2.1. So this code is compatible with both API versions.

…shalling error (#1153) ## What changes are proposed in this pull request? This PR refactors how `APIError` processes error details to fully support the Databricks Error specification. In particular, it provides a clear mechanism to access known error details types while still enabling users to access unknown error details. The core of the PR is the addition of a new `ErrorDetails` type which operates as the union of all error details type. It implements the fact that an error response should only contain at most _one instance of each type_. The code is purposely structured to hide implementation details by (i) exposing `ErrorDetails` via a function, and (ii) separating the exported error details types from their unmarshalling logic. ## How is this tested? 100% unit tests coverage of new code + slight refactor of the test suite.

…rors (#1155) ## What changes are proposed in this pull request? in #769, `poll.Simple` method was added to simplify mocking of waiter objects. However, this only allows testing of happy path where no error is returned. This PR adds the equivalent `poll.SimpleError` method to returns an error instead. ## How is this tested? - [x] Unit test added

…onses (#1151) ## What changes are proposed in this pull request? Introduces logic in the extension for jobs ListRuns call. The extended logic accounts for the new response format of API 2.2. API 2.1 format returns all tasks and job_cluster for each run in the runs list. API 2.2 format truncates tasks and job_cluster to 100 elements. The extended ListRuns logic calls GetRun for each run in the list to populate the full list of tasks and job_clusters. I added logic that reads runs from the list and produces custom iterator struct `expandedRunsIterator` that is supposed to mimic python generators. The goal is to only read necessary elements from the API endpoint and not more. ## How is this tested? Unit tests and manual tests. Manual tests were done in two modes: using API 2.2 and using API 2.1. So this code is compatible with both API versions. --------- Co-authored-by: Renaud Hartert <renaud.hartert@databricks.com>

## What changes are proposed in this pull request? Update OpenAPI spec ## How is this tested? N/A

## What changes are proposed in this pull request? This PR fixes unmarshalling of the `RetryInfo` error detail. Specifically, it properly unmarshal the underlying `google.protobuf.Duration` proto [as a string](https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/duration.proto#L92-L101). Note: we should ultimately consider using the actual proto generated objects instead of this custom code. ## How is this tested? Updated unit tests. NO_CHANGELOG=true

## What changes are proposed in this pull request? Removes the following files: * `.codegen/changelog.md.tmpl`: The new automated tagging does not use this. API Changelogs are now generated at update time and use a separate template. * `.codegen/changelog_config.yml` and `.github/workflow/message.yml`: Changelogs are now manually written and we don't need to enforce the tags in the PR message. ## How is this tested? N/A ## Changelog Skip No user impact NO_CHANGELOG=true

## What changes are proposed in this pull request? This PR updates SDK to the latest OpenAPI spec. This PR also updates the mockery version as the currently used version is not compatible with go 1.24.0. ## How is this tested? Existing Tests

## Release v0.60.0 ### API Changes * Added [w.Forecasting](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/ml#ForecastingAPI) workspace-level service. * Added `ExecuteMessageAttachmentQuery` and `GetMessageAttachmentQueryResult` methods for [w.Genie](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/dashboards#GenieAPI) workspace-level service. * Added `StatementId` field for [dashboards.GenieQueryAttachment](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/dashboards#GenieQueryAttachment). * Added `BudgetPolicyId` field for [serving.CreateServingEndpoint](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/serving#CreateServingEndpoint). * Added `BudgetPolicyId` field for [serving.ServingEndpoint](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/serving#ServingEndpoint). * Added `BudgetPolicyId` field for [serving.ServingEndpointDetailed](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/serving#ServingEndpointDetailed). * Added `CouldNotGetModelDeploymentsException` enum value for [dashboards.MessageErrorType](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/dashboards#MessageErrorType).

## What changes are proposed in this pull request? This PR is a noop that slightly refactors the `CredentialsProvider` and improve doc comments. ## How is this tested? Unit tests (no change) NO_CHANGELOG=true

github-actions · 2025-03-18T17:00:58Z

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-go

Inputs:

PR number: 1179
Commit SHA: 0cdfd54930eac3eccf907fcd88a4da13bc59569a

Checks will be approved automatically on success.

shreyas-goenka · 2025-03-18T17:04:33Z

Closing in favour of #1180

parthban-db and others added 30 commits January 13, 2025 09:58

[Fix] Support Query parameters for all HTTP operations (#1124)

e079db9

## What changes are proposed in this pull request? Support Query parameters for all HTTP operations. Previously, only GET/HEAD/DELETE were supported. ## How is this tested? * Added Unit Tests * Run Integration Tests

[Release] Release v0.56.1 (#1137)

bf617bb

### Bug Fixes * Do not send query parameters when set to zero value ([#1136](#1136)).

[Release] Release v0.58.1 (#1146)

967d063

### Internal Changes * Do not send ForceSendFields as query parameters.

[Internal] Enable Automated Tagging for SDK (#1148)

515359f

## What changes are proposed in this pull request? Enable Automated Tagging for SDK to support faster releases. ## How is this tested? Automated Tagging process has been already enabled in `terraform-provider-databricks` repository.

[Internal] Update OpenAPI spec (#1163)

62307c9

## What changes are proposed in this pull request? Update OpenAPI spec ## How is this tested? N/A

Slightly simplify CredentialsProvider and add doc comments (#1169)

c371ad8

## What changes are proposed in this pull request? This PR is a noop that slightly refactors the `CredentialsProvider` and improve doc comments. ## How is this tested? Unit tests (no change) NO_CHANGELOG=true

Do not close all TCP connections on an HTTP error

0cdfd54

shreyas-goenka temporarily deployed to test-trigger-is March 18, 2025 17:00 — with GitHub Actions Inactive

shreyas-goenka changed the base branch from main to dev/sdk-mod March 18, 2025 17:01

shreyas-goenka temporarily deployed to test-trigger-is March 18, 2025 17:02 — with GitHub Actions Inactive

shreyas-goenka closed this Mar 18, 2025

shreyas-goenka temporarily deployed to test-trigger-is March 18, 2025 17:09 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Do not close all TCP connections on an HTTP error #1179

Do not close all TCP connections on an HTTP error #1179

Uh oh!

shreyas-goenka commented Mar 18, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

shreyas-goenka commented Mar 18, 2025

Uh oh!

Uh oh!

Do not close all TCP connections on an HTTP error #1179

Do not close all TCP connections on an HTTP error #1179

Uh oh!

Conversation

shreyas-goenka commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How is this tested?

Uh oh!

github-actions bot commented Mar 18, 2025

Uh oh!

shreyas-goenka commented Mar 18, 2025

Uh oh!

Uh oh!

shreyas-goenka commented Mar 18, 2025 •

edited

Loading