You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "Kaixuan-Duan (via GitHub)" <gi...@apache.org> on 2026/09/11 20:24:02 UTC
[PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Kaixuan-Duan opened a new pull request, #4308:
URL: https://github.com/apache/fluss/pull/4308
<!--
*Thank you very much for contributing to Fluss - we are happy that you want to help us improve Fluss. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [GitHub issue](https://github.com/apache/fluss/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no issue.
- Name the pull request in the format "[component] Title of the pull request", where *[component]* should be replaced by the name of the component being changed. Typically, this corresponds to the component label assigned to the issue (e.g., [kv], [log], [client], [flink], [rust], [python], [c++], [elixir]). Skip *[component]* if you are unsure about which is the best component.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes.
- Each pull request should address only one issue, not mix up code from multiple issues.
- **Generative AI disclosure:** Indicate whether generative AI tools were used in authoring this PR. If yes, specify the tool below.
- [ ] No generative AI tools used
- [ ] Yes (please specify the tool below)
**(The sections below can be removed for hotfixes or typos)**
-->
<!--
Generated-by: [Tool Name and Version] following [the guidelines](https://github.com/apache/fluss/blob/main/AGENTS.md)
-->
### Purpose
<!-- Linking this pull request to the issue -->
Linked issue: close #4307
<!-- What is the purpose of the change -->
### Brief change log
<!-- Please describe the changes made in this pull request and explain how they address the issue -->
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "platinumhamburg (via GitHub)" <gi...@apache.org>.
platinumhamburg commented on code in PR #4308:
URL: https://github.com/apache/fluss/pull/4308#discussion_r4033897654
##########
fluss-server/src/main/java/org/apache/fluss/server/metrics/group/TabletServerMetricGroup.java:
##########
@@ -90,6 +91,10 @@ public class TabletServerMetricGroup extends AbstractMetricGroup {
private volatile long sharedWriteBufferCapacity;
+ /** Ledger shared by all KV pre-write buffers, serving as the single accounting source. */
+ private final KvPreWriteBufferMemoryLedger kvPreWriteBufferMemoryLedger =
Review Comment:
Could we simplify this to a single shared AtomicLong owned by KvManager, tracking only memory bytes, and remove the entry count and the separate KvPreWriteBufferMemoryLedger class?
KvManager already owns the shared KV resources and tablet lifecycle, so it is a more natural owner of this accounting state. The shared counter can be passed through KvTablet to each buffer, with each buffer retaining its local byte count for release on close. TabletServerMetricGroup would only register a gauge that reads the counter. This also reduces per-entry atomic updates and keeps memory accounting ownership outside the metrics object.
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/prewrite/KvPreWriteBuffer.java:
##########
@@ -322,6 +362,59 @@ public void abortAllPrepared() {
}
}
+ /**
+ * Adds an entry to the incrementally maintained memory estimate and entry count. An entry
+ * without a previous version is the latest version of a new key and thus adds one map node.
+ */
+ private void addToAccounting(KvEntry entry) {
+ long bytes = entryBytes(entry.getKey(), entry.getValue());
+ pendingFlushBytes += bytes;
+ long accountedBytes =
+ bytes
+ + PER_ENTRY_OVERHEAD_BYTES
+ + (entry.previousEntry == null ? PER_MAP_NODE_OVERHEAD_BYTES : 0L);
+ memoryUsageBytes += accountedBytes;
+ entryCount++;
+ memoryLedger.add(accountedBytes, 1);
+ }
+
+ /**
+ * Removes an entry from the key map and deducts it from the incrementally maintained memory
+ * estimate and entry count. Returns whether the entry was the latest version of its key and
+ * thus removed from the map.
+ */
+ private boolean removeFromMapAndAccounting(KvEntry entry) {
+ long bytes = entryBytes(entry.getKey(), entry.getValue());
+ pendingFlushBytes -= bytes;
+ boolean removedFromMap = kvEntryMap.remove(entry.getKey(), entry);
+ long accountedBytes =
+ bytes
+ + PER_ENTRY_OVERHEAD_BYTES
+ + (removedFromMap ? PER_MAP_NODE_OVERHEAD_BYTES : 0L);
+ memoryUsageBytes -= accountedBytes;
+ entryCount--;
+ memoryLedger.subtract(accountedBytes, 1);
Review Comment:
This subtracts the map-node overhead when removing the latest version, but truncateTo() may then restore an older version of the same key to the map without restoring that accounting.
Inserting v1 and v2 for the same key, truncating v2, and then flushing v1 leaves both the local and global memory counters at -32 with an empty buffer. A subsequent close() passes this negative value to ledger.subtract() and throws IllegalArgumentException. KvTablet has already set isClosed=true at that point, but has not closed the scheduler or RocksDB, so cleanup is interrupted and subsequent close calls cannot complete it.
Please preserve or restore the map-node accounting when reinstating an older version, and add a regression test covering same-key versions through truncate → flush → close.
BTY, this updates the shared atomic counter for every entry in the flush and truncate loops. Could we keep updating the local accounting per entry, accumulate the bytes actually released by the operation, and subtract them from the global counter in one update? This would reduce atomic updates and contention across tablets.
Please also handle partial completion on exceptions: entries already removed must still be accounted for. Once the operation completes, the global counter should equal the sum of the local buffer counters; close should release only the remaining accounting and remain idempotent. A brief conservative overestimate while the batched decrement is pending is acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "Kaixuan-Duan (via GitHub)" <gi...@apache.org>.
Kaixuan-Duan commented on code in PR #4308:
URL: https://github.com/apache/fluss/pull/4308#discussion_r4067847961
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/prewrite/KvPreWriteBuffer.java:
##########
@@ -181,8 +207,8 @@ private void doPut(ChangeType changeType, Key key, Value value, long lsn) {
allKvEntries.addLast(kvEntry);
// update the max lsn
maxLogSequenceNumber = lsn;
- // track accumulated bytes for flush budget gating
- pendingFlushBytes += entryBytes(key, value);
+ // update the accounting for flush budget gating and metrics
+ addToAccounting(kvEntry);
Review Comment:
Done — the put path is now symmetric with flush/truncate: the delta is accumulated locally per record and publishPendingAccountingDelta() applies it in a single addAndGet per write batch, called in a try/finally at the end of putAsLeader (and once after log replay). close() settles the remaining accounting in one update, including any unpublished delta.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "Kaixuan-Duan (via GitHub)" <gi...@apache.org>.
Kaixuan-Duan commented on code in PR #4308:
URL: https://github.com/apache/fluss/pull/4308#discussion_r4035257563
##########
fluss-server/src/main/java/org/apache/fluss/server/metrics/group/TabletServerMetricGroup.java:
##########
@@ -90,6 +91,10 @@ public class TabletServerMetricGroup extends AbstractMetricGroup {
private volatile long sharedWriteBufferCapacity;
+ /** Ledger shared by all KV pre-write buffers, serving as the single accounting source. */
+ private final KvPreWriteBufferMemoryLedger kvPreWriteBufferMemoryLedger =
Review Comment:
Done. Removed the KvPreWriteBufferMemoryLedger class and the entry-count metric entirely. The accounting state is now a single AtomicLong owned by KvManager, passed through KvTablet to each buffer, with each buffer retaining its local byte count for release on close. TabletServerMetricGroup only registers a gauge that reads the counter via a supplier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #4308:
URL: https://github.com/apache/fluss/pull/4308#issuecomment-5658524386
looks duplicate with https://github.com/apache/fluss/pull/3533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on PR #4308:
URL: https://github.com/apache/fluss/pull/4308#issuecomment-5660480359
> I've looked at both https://github.com/apache/fluss/pull/4308 and https://github.com/apache/fluss/pull/3533. IMO, the way https://github.com/apache/fluss/pull/4308 calculates actual memory usage reflects the true memory footprint — it factors in the real overhead of Java objects, which I believe is the correct approach.
make sense, please go ahead @Kaixuan-Duan . And I will close the #3533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "platinumhamburg (via GitHub)" <gi...@apache.org>.
platinumhamburg commented on PR #4308:
URL: https://github.com/apache/fluss/pull/4308#issuecomment-5658883925
> looks duplicate with #3533
I've looked at both #4308 and #3533. IMO, the way #4308 calculates actual memory usage reflects the true memory footprint — it factors in the real overhead of Java objects, which I believe is the correct approach.
@zuston, do you still plan to move forward with #3533? If so, I'd suggest adjusting it to follow the approach taken in this PR.
Additionally, I think the unified memory accounting introduced in #3751 should be incorporated into this PR. To be more specific: we need a truly accurate global memory usage ledger for KvPrewriteBuffer, maintained by atomic variables. Our future backpressure decisions will depend on it, and the current metrics should also be built on the same source of truth — rather than being the sum of non-atomic samples collected from multiple maps by the metrics framework.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "Kaixuan-Duan (via GitHub)" <gi...@apache.org>.
Kaixuan-Duan commented on code in PR #4308:
URL: https://github.com/apache/fluss/pull/4308#discussion_r4035274598
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/prewrite/KvPreWriteBuffer.java:
##########
@@ -322,6 +362,59 @@ public void abortAllPrepared() {
}
}
+ /**
+ * Adds an entry to the incrementally maintained memory estimate and entry count. An entry
+ * without a previous version is the latest version of a new key and thus adds one map node.
+ */
+ private void addToAccounting(KvEntry entry) {
+ long bytes = entryBytes(entry.getKey(), entry.getValue());
+ pendingFlushBytes += bytes;
+ long accountedBytes =
+ bytes
+ + PER_ENTRY_OVERHEAD_BYTES
+ + (entry.previousEntry == null ? PER_MAP_NODE_OVERHEAD_BYTES : 0L);
+ memoryUsageBytes += accountedBytes;
+ entryCount++;
+ memoryLedger.add(accountedBytes, 1);
+ }
+
+ /**
+ * Removes an entry from the key map and deducts it from the incrementally maintained memory
+ * estimate and entry count. Returns whether the entry was the latest version of its key and
+ * thus removed from the map.
+ */
+ private boolean removeFromMapAndAccounting(KvEntry entry) {
+ long bytes = entryBytes(entry.getKey(), entry.getValue());
+ pendingFlushBytes -= bytes;
+ boolean removedFromMap = kvEntryMap.remove(entry.getKey(), entry);
+ long accountedBytes =
+ bytes
+ + PER_ENTRY_OVERHEAD_BYTES
+ + (removedFromMap ? PER_MAP_NODE_OVERHEAD_BYTES : 0L);
+ memoryUsageBytes -= accountedBytes;
+ entryCount--;
+ memoryLedger.subtract(accountedBytes, 1);
Review Comment:
Thanks for catching this. All three points addressed:
- truncateTo() now restores the map-node accounting when reinstating an older version, so the truncate → flush → close sequence brings both counters back to zero instead of −32.
- The flush and truncate loops update only the local accounting per entry and apply the accumulated release to the shared counter in a single addAndGet at the end of the operation.
- Both loops run in try/finally, so entries already removed before an exception are still accounted for; after completion the global counter equals the sum of the local counters, and close releases only the remaining accounting and stays idempotent.
Added two regression tests: testSameKeyVersionAccountingAcrossTruncateFlushClose (same-key versions through truncate → flush → close) and testTruncatePartialCompletionStillReleasesAccounting (partial completion on exception).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "Kaixuan-Duan (via GitHub)" <gi...@apache.org>.
Kaixuan-Duan commented on PR #4308:
URL: https://github.com/apache/fluss/pull/4308#issuecomment-5694396061
@platinumhamburg @zuston Thanks for the direction, and for closing #3533
The unified ledger is now part of this PR: KvPreWriteBufferMemoryLedger is a tabletserver-wide singleton with two AtomicLongs (estimated memory usage including per-entry object overhead, and entry count). Every pre-write buffer reports its accounting deltas to the ledger atomically on the write path (add on put, subtract on flush/truncate), and closes release their remaining accounting, so the ledger stays exact across tablet lifecycles. Both gauges now read directly from this single ledger — the previous per-tablet sampling and summation in KvManager is removed. No backpressure behavior is introduced; the watermarks and rejection logic from #3751 can be built on top of this ledger in a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [server] Add pre-write buffer memory metrics for primary key tables [fluss]
Posted by "platinumhamburg (via GitHub)" <gi...@apache.org>.
platinumhamburg commented on code in PR #4308:
URL: https://github.com/apache/fluss/pull/4308#discussion_r4062023145
##########
fluss-server/src/main/java/org/apache/fluss/server/kv/prewrite/KvPreWriteBuffer.java:
##########
@@ -181,8 +207,8 @@ private void doPut(ChangeType changeType, Key key, Value value, long lsn) {
allKvEntries.addLast(kvEntry);
// update the max lsn
maxLogSequenceNumber = lsn;
- // track accumulated bytes for flush budget gating
- pendingFlushBytes += entryBytes(key, value);
+ // update the accounting for flush budget gating and metrics
+ addToAccounting(kvEntry);
Review Comment:
The put path updates the server-wide shared `AtomicLong` once per mutation, while `completeFlush`/`truncateTo` were already reworked to batch their release into a single `addAndGet` per operation. This leaves `addToAccounting` as the only place still doing a per-record RMW on a counter shared by every KV tablet on the server, so concurrent writers across tablets contend on the same cache line on the hot write path.
Suggest making the put path symmetric with flush/truncate: keep updating the local `memoryUsageBytes` per entry, and publish the accumulated delta to the shared counter once at a batch boundary (the write already holds `kvLock`). A brief conservative under-count between batch boundaries is acceptable for an observability metric and is consistent with the flush/truncate semantics already in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org