[server] Expose KV WAL memory pool metrics for primary key tables - #4248
[server] Expose KV WAL memory pool metrics for primary key tables#4248Kaixuan-Duan wants to merge 1 commit into
Conversation
|
@platinumhamburg Could you please review this PR when you have time? |
platinumhamburg
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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.
Purpose
Linked issue: close #4249
The server-wide WAL memory pool (
server.buffer.memory-size, used by all primary keytable 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
walMemoryPoolUsage/walMemoryPoolCapacity/walMemoryPoolWaitingThreadsgauges to the tabletserver metric group.
LazyMemorySegmentPoolexposesusedPages(); waiting threads reuse the existingqueued(), mirroring the client-sidebufferWaitingThreadsmetric.KvManagerright after the pool is created, following the existingsetSharedWriteBufferMetricspattern.Tests
TabletServerMetricGroupTest#testWalMemoryPoolMetrics: verifies gauge registrationand that usage tracks page allocation/release dynamically.
API and Format
No changes.
Documentation
monitor-metrics.md: document the three new tabletserver gauges.