8000 fix(memory): stop the process-lifetime leaks -- fixed-cardinality DDL metrics, metrics registry released on stop and reinitialize, worker connections closed with the pool, one binlog-position gauge child (Spec 10.03 section 3.4, Spec 01.01 section 3.3 step 4a) by minguyen9988 · Pull Request #1506 · Altinity/clickhouse-sink-connector · GitHub
[go: up one dir, main page]
Skip to content

fix(memory): stop the process-lifetime leaks -- fixed-cardinality DDL metrics, metrics registry released on stop and reinitialize, worker connections closed with the pool, one binlog-position gauge child (Spec 10.03 section 3.4, Spec 01.01 section 3.3 step 4a) - #1506

Merged
minguyen9988 merged 2 commits into
2.11.0from
fix-metrics-and-restart-leaks
Sep 26, 2026
Merged

fix(memory): stop the process-lifetime leaks -- fixed-cardinality DDL metrics, metrics registry released on stop and reinitialize, worker connections closed with the pool, one binlog-position gauge child (Spec 10.03 section 3.4, Spec 01.01 section 3.3 step 4a)#1506
minguyen9988 merged 2 commits into
2.11.0from
fix-metrics-and-restart-leaks

Conversation

@minguyen9988
@minguyen9988 minguyen9988 commented Sep 26, 2026
Copy link
Copy Markdown
Collaborator

Independent of #1504 and #1505 (branches from the 2.11.0 head directly).

Defects

Three structures grew for the life of the process on a fixed heap, with no relation to the current workload:

  1. Metrics.updateDdlMetrics tagged the clickhouse.sink.ddl counter with the DDL TEXT and a wall-clock TIMESTAMP. Every DDL event therefore registered a new series that was never removed: on a source that refreshes its views thousands of times a day that is tens of thousands of retained series (each holding the statement text) per week, and every /metrics scrape renders all of them.
  2. Metrics.initialize runs on every engine start in the process (REST restart/start, the restart monitor) and built a fresh CollectorRegistry + PrometheusMeterRegistry each time without closing the previous one. The JvmGcMetrics binder installs GC-MXBean notification listeners that keep the old registry -- and every series ever registered in it, including all of (1) -- reachable until the JVM exits.
  3. A discarded ClickHouseBatchRunnable never closed the connections in its per-database map nor its system connection. Every engine restart shuts the pool down and schedules new workers, so each restart leaked thread.pool.size x databases connections.
  4. (minor) The binlog position gauge is labelled by binlog file and kept one child per file since start.

Rule (spec 10.03 section 3.4; spec 01.01 section 3.3 step 4a)

  • Label values come from bounded sets. The DDL counter carries only fail=true/false (two series); the statement is in the log at INFO, the timestamp is the scrape's.
  • Metrics.stop() closes the JvmGcMetrics binder and the meter registry and drops the references; Metrics.initialize() stops a previous registry before opening a new one.
  • ClickHouseBatchRunnable.closeConnections() closes and forgets the per-database and system connections (a failing close is logged and skipped, the call is idempotent); DebeziumChangeEventCapture keeps the runnables it scheduled and calls it in stop() right after the pool has terminated, before the FIFO reset.
  • The binlog position gauge removes the child of the previous file when the file changes.

Checks on this branch

  • validate_specs: PASSED (63 specifications, 644 citations).
  • sink-connector: 15/15 -- new MetricsLifecycleTest 3/3 (fixed DDL cardinality across distinct statements/timestamps; one gauge child across a rotation; registry closed on stop and on reinitialize), new ClickHouseBatchRunnableCloseConnectionsTest 1/1 (closes and forgets per-database + system connections, skips a refusing one, idempotent); MetricsContentTypeTest 3/3, SystemConnectionRefreshTest 4/4, ClickHouseBatchRunnableDatabaseBootstrapTest 3/3, ClickHouseBatchRunnableTest 1/1. BUILD SUCCESS.
  • sink-connector-lightweight: 12/12 -- StoppedEngineRetiresHandoffsTest 3/3, DeadWorkerRetryIsTerminalTest 3/3, EngineRestartFifoResetTest 6/6. BUILD SUCCESS.

@minguyen9988
fix(memory): stop the three process-lifetime leaks -- the DDL metrics…
44e9755 
… counter carried the statement text and a wall-clock timestamp as tags (one series per DDL event, never removed), each engine restart built a new metrics registry without closing the old one (its JvmGcMetrics listeners kept every series reachable), and discarded workers never closed their per-database and system connections; also one binlog-position gauge child instead of one per rotation (Spec 10.03 section 3.4, Spec 01.01 section 3.3 step 4a)
@minguyen9988
test(memory): the close-connections test records through static state…
ea4d5ed 
… -- the superclass constructor opens the system connection before the subclass's field initializers run
@minguyen9988
minguyen9988 merged commit 5e4f9b7 into 2.11.0 Sep 26, 2026
11 checks passed
@minguyen9988
minguyen9988 deleted the fix-metrics-and-restart-leaks branch September 26, 2026 06:26
This was referenced Sep 26, 2026
Merged
Merged
Merged
minguyen9988 added a commit that referenced this pull request Sep 27, 2026
@minguyen9988
fix(metrics): a counter update with no open registry is dropped, not …
f69fde9 
…thrown -- since the registry is released on stop() (#1506), updateCounters/updateErrorCounters/updateDdlMetrics registered through a null reference between stop() and the next initialize() and threw NullPointerException out of the write path (Spec 10.03 section 3.4 item 4) (#1517)

The three counter updates now check that metrics are enabled and the meter
registry is present and not closed before registering; an increment that
arrives with no registry has nowhere to be scraped from and is dropped.
This restores the behaviour the leaked registry used to provide by accident,
without the leak.

Observed: the sink-connector module runs its whole suite in one JVM
(forkCount=0). When MetricsContentTypeTest / MetricsLifecycleTest run before
db.batch.*, eleven PreparedStatementExecutor tests fail with
NullPointerException: Cannot invoke MeterRegistry.counter(Meter$Id) because
"registry" is null. The order depends on the file system, so CI stayed green
while a local run went red.

Test: MetricsLifecycleTest.counterUpdatesAfterStopAreDroppedNotThrown --
red with NullPointerException when the guard is removed from any of the three
updates, green with it; the next initialize() counts again from zero.
Spec: 10.03 section 3.4 item 4, section 5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@minguyen9988
0