镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix(client-v2): record connection times only when metrics are enabled - #3309

Open
pcbeingused333 wants to merge 1 commit into
ClickHouse:mainfrom
pcbeingused333:c1692-times-queue
Open

pcbeingused333 wants to merge 1 commit into
ClickHouse:mainfrom
pcbeingused333:c1692-times-queue

Conversation

@pcbeingused333

@pcbeingused333 pcbeingused333 commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

MeteredManagedHttpClientConnectionFactory.createConnection adds the connection creation time to a ConcurrentLinkedQueue on every new pooled connection, but the queue is only drained by getTime(), which only the Micrometer gauge calls, and that gauge is registered only when a metrics registry is configured. Without a registry (the default, and always in jdbc-v2), one queue node is retained per new connection for the life of the Client.

The factory now records times only after enableMetrics() is called, which HttpAPIClientHelper does only when a metrics registry is present, right before the gauge is bound. Behaviour with a registry is unchanged.

Closes #3253

Checklist


This PR was prepared with the assistance of an AI coding tool (Claude); the change was reviewed before submission.


Note

Low Risk
Targeted leak fix behind a flag; metrics-enabled behaviour is unchanged and covered by new unit tests.

Overview
Fixes a memory leak in pooled HTTP clients when no metrics registry is configured (the default for client-v2 and jdbc-v2).

MeteredManagedHttpClientConnectionFactory previously appended every new connection’s creation duration to an internal queue, but that queue is drained only via getTime() when Micrometer binds the connect-time gauge. Without a registry, entries were never removed and grew with each new connection for the client’s lifetime.

The factory now gates timing behind enableMetrics(), which HttpAPIClientHelper calls only when a metrics registry is present. createConnection skips queueing when metrics are off; behaviour with Micrometer unchanged. Unit tests in MeteredConnectionFactoryTest cover both paths, and history/latest/3253.md documents the fix for #3253.

Reviewed by Cursor Bugbot for commit 32c65d7. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 32c65d7. Configure here.

PoolingHttpClientConnectionManager phccm = connMgrBuilder.build();
poolControl = phccm;
if (metricsRegistry != null) {
connectionFactory.enableMetrics();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics stay enabled after bind failure

Medium Severity

enableMetrics() runs before the Micrometer binders and is not turned off when registration fails. The client then keeps enqueueing connection times with no gauge calling getTime(), so the unbounded times queue this change addresses can still grow for the lifetime of the Client.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 32c65d7. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[client-v2] Connect-time queue of the pooled connection factory grows without limit when no metrics registry is set

1 participant