Skip to content

[server] Expose KV WAL memory pool metrics for primary key tables - #4248

Open
Kaixuan-Duan wants to merge 1 commit into
apache:mainfrom
Kaixuan-Duan:wal-memory-pool-metrics
Open

[server] Expose KV WAL memory pool metrics for primary key tables#4248
Kaixuan-Duan wants to merge 1 commit into
apache:mainfrom
Kaixuan-Duan:wal-memory-pool-metrics

Conversation

@Kaixuan-Duan

@Kaixuan-Duan Kaixuan-Duan commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: close #4249

The server-wide WAL memory pool (server.buffer.memory-size, used by all primary key
table buckets to build changelog WAL) is currently unobservable: no metric exposes its
usage or allocation contention, which made pool exhaustion incidents hard to diagnose.

Brief change log

  • Add walMemoryPoolUsage / walMemoryPoolCapacity / walMemoryPoolWaitingThreads
    gauges to the tabletserver metric group.
  • LazyMemorySegmentPool exposes usedPages(); waiting threads reuse the existing
    queued(), mirroring the client-side bufferWaitingThreads metric.
  • Registered by KvManager right after the pool is created, following the existing
    setSharedWriteBufferMetrics pattern.

Tests

  • TabletServerMetricGroupTest#testWalMemoryPoolMetrics: verifies gauge registration
    and that usage tracks page allocation/release dynamically.

API and Format

No changes.

Documentation

monitor-metrics.md: document the three new tabletserver gauges.

@Kaixuan-Duan

Copy link
Copy Markdown
Contributor Author

@platinumhamburg Could you please review this PR when you have time?

@platinumhamburg platinumhamburg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The usage and capacity calculations appear correct, but the monitoring read path should avoid the allocation lock, and the metric group should depend on supplied values rather than the concrete pool. Keep this change focused on usage and capacity, clarify their meaning, and align the tests with that boundary.


/** Returns the number of pages currently allocated to callers. */
public int usedPages() {
return inLock(lock, () -> pageUsage);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we make this accessor a lock-free read? The new gauge takes the same exclusive lock as page allocation and return, and allocation also performs heap allocation while holding that lock. This couples metric collection to the production allocation path in both directions: collection can contend with writers, and allocation can delay collection. An instantaneous usage value is sufficient here; we do not need a consistent snapshot across gauges. Please make pageUsage volatile and read it directly in usedPages(), while retaining the existing lock around accounting updates. The accessor should remain safe after pool closure so an in-flight collection can finish without throwing. Do not reset the accounting counter merely for monitoring during close, since pages can still be returned afterward.

* @param walMemoryPool the server-wide WAL memory segment pool
*/
public void setWalMemoryPoolMetrics(LazyMemorySegmentPool walMemoryPool) {
LazyMemorySegmentPool pool = checkNotNull(walMemoryPool, "walMemoryPool must not be null");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we narrow this to registerWalMemoryPoolMetrics(LongSupplier usageSupplier, long capacity)? The metric group only needs a byte count and a fixed capacity, but currently depends on LazyMemorySegmentPool and knows how to convert pages into bytes. KvManager should bind the data source and perform the conversion; the metric group should only register the gauges. Capture a local pool variable in the supplier rather than implicitly capturing the entire manager. Register the supplier directly with the gauge without an additional supplier field. The existing metric-group shutdown unregisters and clears the gauges, and a safe numeric accessor tolerates any in-flight read. No separate cleanup callback or weak reference is needed. register also expresses the one-time operation accurately: duplicate metric registration retains the original gauge, so this method should not imply that it replaces an existing data source.

LazyMemorySegmentPool pool = checkNotNull(walMemoryPool, "walMemoryPool must not be null");
gauge(MetricNames.WAL_MEMORY_POOL_USAGE, () -> (long) pool.usedPages() * pool.pageSize());
gauge(MetricNames.WAL_MEMORY_POOL_CAPACITY, pool::totalSize);
gauge(MetricNames.WAL_MEMORY_POOL_WAITING_THREADS, pool::queued);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please keep this PR focused on WAL pool usage and capacity and omit walMemoryPoolWaitingThreads. I do not see sufficient operational value in the waiting-thread gauge for this change to justify adding it to the exposed metric surface. Remove its constant, registration, assertions, and documentation entry, including the corresponding table row-span adjustment. Leave the existing queued() behavior unchanged and omit the Javadoc added to it by this PR. This is a scope recommendation, not a claim that queued() returns an incorrect value.

public static final String WAL_MEMORY_POOL_USAGE = "walMemoryPoolUsage";

/** Total capacity of the WAL memory pool for primary key tables in this server (bytes). */
public static final String WAL_MEMORY_POOL_CAPACITY = "walMemoryPoolCapacity";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we name these kvWalMemoryPoolUsage and kvWalMemoryPoolCapacity? This pool serves the KV write path, while the tabletserver metric group contains both Log and KV metrics. A kv prefix would make that distinction explicit and align with names such as kvFlushPerSecond and kvBackpressureMaxPressure. The Usage/Capacity suffixes already match the existing memory metrics.
Please also move these constants out of the RocksDB metrics section into a separate server-level KV WAL memory pool section. This is a Fluss WAL buffer, not a RocksDB resource. The registration method could follow the same naming: registerKvWalMemoryPoolMetrics.

@platinumhamburg platinumhamburg changed the title [server] Expose WAL memory pool metrics for primary key tables [server] Expose KV WAL memory pool metrics for primary key tables Sep 8, 2026
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.

[server] Add WAL memory pool metrics for primary key tables

2 participants