lakectl: share a single API client across a command invocation - #10509
Closed
idanovo wants to merge 1 commit into
Closed
lakectl: share a single API client across a command invocation#10509idanovo wants to merge 1 commit into
idanovo wants to merge 1 commit into
Conversation
getClient() built a fresh apigen client — each with its own cloned http.Transport and therefore its own connection pool — on every call. Since sendStats runs in PersistentPreRun with its own getClient() and the command body calls getClient() again, every lakectl invocation opened two TCP+TLS connections to the server where one suffices. Memoize the client with sync.Once (same pattern as getTokenOnce): all callers now share one client and one connection pool, so an invocation dials the server once. Config is finalized in preRunCmd before the first getClient() call, so behavior is otherwise unchanged. At high invocation rates (pipelines spawning lakectl per file) this halves the new-connection load on the server's load balancer and on host connection tracking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or 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
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.
Change Description
Background
Every call to
getClient()constructs a newapigenclient, and each one gets its ownhttp.Clientwith a clonedhttp.Transport— i.e. its own connection pool. Two places construct clients during a single lakectl run:sendStatsinPersistentPreRun(the usage-stats POST sent on every invocation), andfs cat,fs download, etc.).Because the two clients never share a pool, every lakectl invocation opens two TCP+TLS connections to the lakeFS server where one would do. For interactive use this is invisible; for fleets that spawn lakectl per work item it doubles the connection load on the server side. We observed this in production with a pipeline invoking lakectl ~1,500 times/second: the NLB in front of lakeFS recorded almost exactly 2 new flows per invocation, contributing to connection-tracking exhaustion on the hosts behind it. (Ironically,
getHTTPClientWithRetryConfigalready tunesMaxIdleConnsPerHostto avoid TIME_WAIT churn — a tuning that only pays off if callers actually share the transport.)Bug Fix
getClient()builds a new client + transport per call; the pre-run stats reporter and the command each call it once.sync.Once(same pattern asgetTokenOnce), renaming the constructor tonewAPIClient(). All callers now share one client and one connection pool, so an invocation dials once. Config is finalized inpreRunCmdbefore the firstgetClient()call, so construction inputs are identical to today's second call — no behavior change beyond connection reuse.Testing Details
go build ./cmd/lakectl/...,go vet ./cmd/lakectl/...,go test ./cmd/lakectl/...pass.TestMaybeWarnEnterpriseswitches the server endpoint between subtests; added aresetAPIClientCache()test helper and per-subtest reset so the memoized client follows the endpoint change.Breaking Change?
No. Same requests, same auth, same User-Agent — just over one connection instead of two.
Additional info
For a process that makes a single API call (the most common CLI case: one command + one stats POST), this halves connections per invocation. It does not change the process-per-invocation cost itself — callers invoking lakectl once per file may still want to batch — but it makes the floor 1 connection instead of 2 for every lakectl user.
🤖 Generated with Claude Code