Register each User-Agent entry once to stop per-connection latency growth - #1714
vuanhphung wants to merge 2 commits into
Conversation
UserAgent.withOtherInfo appends to a static list on every call, and UserAgent.asString formats the whole list under a global lock on every HTTP request. Calling it from setUserAgent on each connection made request latency grow with the number of connections opened in the JVM. Register each entry once and cap distinct UserAgentEntry values at 64. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-scoped fix for the per-connection User-Agent list growth, with correct dedup/cap logic (duplicates don't inflate the counter, SDK append precedes set insertion, method is synchronized) and good test coverage. One low note: the 64-entry cap is global per-JVM, so high-cardinality multi-tenant JVMs could silently drop customer attribution after 64 distinct entries.
Log once at WARN when the 64-entry cap is first hit and note the cap in the changelog. Verify withOtherInfo call counts with a static mock instead of reflecting into UserAgent.otherInfo, reset the registry around each test, and add an SEA test past the cap. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-scoped, well-tested fix for the per-connection User-Agent growth, with sound synchronization, correct cap/dedup ordering, and matching changelog + unit coverage. Only one low, edge-case note about the dedup Set sharing a namespace between client-type and customer entries. The known limitation that customer entries remain JVM-global (so the cap is cross-workspace, and Thrift carries other connections' entries) is already acknowledged in the PR description as a follow-up.
| } | ||
|
|
||
| private static synchronized void registerOtherInfo( | ||
| String key, String value, boolean isCustomerEntry) { |
There was a problem hiding this comment.
🔵 Low — The dedup Set and the registeredCustomerEntries counter are shared across both client-type and customer entries. Because the key is key + "/" + value, a customer who passes Java/THttpClient or Java/SQLExecHttpClient as their UserAgentEntry would collide with the client-type entry: it would be deduped against (or dedupe) the client entry and never increment the customer count. This is an unlikely edge case (customer prefixes are typically their app name, not Java), and the net effect is benign, but it means the cap accounting is keyed on the full key/value string rather than being namespaced per entry type. Not blocking — flagging for awareness.
This branch has not been deployed
1 participant
Description
UserAgentManager.setUserAgent()runs on everyDriver.connect()and calls the SDK'sUserAgent.withOtherInfo(), which appends to a static list without deduplicating.UserAgent.asString()then formats that entire list under a global lock on every HTTP request, from both the Thrift transport and the SDKApiClient. In a long-running JVM that opens a connection per query, request latency and lock contention grow with every connection the JVM has ever opened.This registers each
key/valueentry with the SDK once per JVM. DistinctUserAgentEntryvalues are capped at 64, so applications that pass a unique value per connection stay bounded. The driver logs once at WARN when the cap is first hit; SQL Execution API requests still send their own entry, and client-type entries are not capped. Below the cap, the rendered User-Agent header is unchanged, sinceasString()already deduplicates.Testing
Unit tests
UserAgentManagerTest: 1,000 repeatedsetUserAgent()calls register each entry with the SDK exactly once (verified withmockStatic(UserAgent.class, CALLS_REAL_METHODS)), and the entry still appears in the header. Distinct customer entries stop at 64 while client-type entries are still registered.DatabricksSdkClientUserAgentTest: past the cap, an SQL Execution API request still carries its ownUserAgentEntryright afteros/.UserAgentManagerTest,UserAgentOrderingTest,DatabricksSdkClientUserAgentTestandDatabricksSdkClientTestpass.Reproduction against a SQL warehouse
A single long-lived JVM runs 16 threads, each looping
Driver.connect()→SELECT 1→close()for 2 minutes (~3.7k connections), with OAuth token passthrough, aUserAgentEntry, and JFR (-XX:StartFlightRecording=settings=profile). Every 10 s it logs per-phase latency, the size of the SDK'sUserAgent.otherInfolist, and the time of onegetUserAgentString()call. A control run trimsotherInfoby reflection to separate its effect from warehouse, network and TLS variance.UserAgent.asStringA third 3.4.1 run seeded the list with 40k simulated prior connections. Connect p50 rose to ~2 s, close to ~1.3 s and execute to ~0.8 s, against a ~245 / 81 / 160 ms baseline; 97% of worker CPU samples were in
asString, and threads spent 803 s in monitor waits on the list over 60 s.Telemetry Errors
Additional Notes to the Reviewer
UserAgent.withOtherInfo.mvn clean package; an incremental build reused stale classes fromassembly-uber/target.This pull request and its description were written by Isaac.