Conversation
…h in the FE image BinlogManager.writeTBinlogToStream persists every binlog record of the FE image with the capacity of its serialization buffer instead of the number of bytes actually serialized. libthrift's TMemoryBuffer.getArray() returns the backing array of its TByteArrayOutputStream, allocated at BUFFER_SIZE (16KB) and grown by doubling, while length() is the number of valid bytes. Verified identical on libthrift 0.16.0 and 0.24.0. As a result every record occupies at least 4 + 16384 bytes in the image whatever its real size: a DUMMY record, one per database and one per table that has produced a binlog, serializes to 52 bytes, so it should take 56 bytes of stream and instead takes 16388 (292x). Records larger than 16KB are padded to the next doubled capacity, up to 2x. On load, readTBinlogFromStream then allocates a byte[] of that padded length for every record, which happens twice per checkpoint (Checkpoint.doCheckpoint loads the image, saves the new one, and loads it again to validate it) plus once on startup, and the master pushes the whole image to every non-master frontend after each checkpoint. Write buffer.length() bytes instead. Measured on a 105020 record workload, over 6 runs on a shared host: image 1.60GB -> 41.16MB (39.9x, byte identical in every run), read side allocation 1.76GB -> 162.71MB (11.1x), write 14.9-16.6s -> 0.9-1.2s, read 2.1-2.8s -> 0.08-0.17s. Write side allocation is unchanged: this does not touch the TMemoryBuffer growth chain, only what is written out of it. Measuring each record type with its own record class and the GsonUtils.GSON that BinlogManager uses: DUMMY is 56 B, a typical single partition UPSERT 478-974 B, and a CREATE_TABLE stays under 16KB up to ~293 columns. An UPSERT only reaches 16KB when one transaction touches ~92 partitions or 500-1000 tablets. The exception is ADD_PARTITION, which embeds the whole Partition -> Tablet -> Replica tree and crosses 16KB at ~28 buckets x 3 replicas; those records are still padded, just to the next doubled capacity, so the fix is worth ~2x for them rather than ~40x. The record framing is unchanged (int length + payload) so no FeMetaVersion bump is needed, and both formats were read back by the BinlogManager of a released Doris 4.0.5 binary with identical payloads. Old images stay readable because the padding sits after the thrift STOP field and is ignored by TBinlog.read(). Present since apache#17881 (May 2023), and the same code is on the release branches. Only materializes when enable_feature_binlog is true; with the default false, Env.saveBinlogs returns before writing anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27237 ms |
Contributor
TPC-DS: Total hot run time: 151852 ms |
Contributor
ClickBench: Total hot run time: 23.83 s |
w41ter
approved these changes
Sep 21, 2026
yiguolei
approved these changes
Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: none (found during internal investigation)
Problem Summary:
BinlogManager.writeTBinlogToStreampersists every binlog record of the FE image with the capacity of its serialization buffer instead of the number of bytes actually serialized:libthrift's
TMemoryBuffer.getArray()returns the backing array of itsTByteArrayOutputStream— allocated atBUFFER_SIZEand grown by doubling — whilelength()is the number of valid bytes. Verified on both libthrift 0.16.0 and 0.24.0 (the version master uses today); the behaviour is identical.Consequences:
4 + 16384bytes in the image, whatever its real size. ADUMMYrecord — one per database and one per table that has produced a binlog, seeDBBinlog.getAllBinlogs()— serializes to 52 bytes, so it should take 56 bytes of stream and instead takes 16388, 292x;readTBinlogFromStreamallocates abyte[]of that padded length for every record.Checkpoint.doCheckpoint()loads the image, saves the new one and then loads it again to validate it, so this happens twice per checkpoint, plus once on FE startup;Write
buffer.length()bytes instead.The bug has been present since #17881 (May 2023). This PR targets
master; the same code is present on the release branches, so maintainers may want to consider it there as well.It only materializes when
enable_feature_binlog = true; with the defaultfalse,Env.saveBinlogsreturns before writing anything, so this change is a no-op for those clusters.Measurements
1. Real FE images. The
binlogsmodule of FE images from three binlog-enabled test clusters, parsed offline (record count, bytes on disk, bytes actually needed):DUMMY, 1DROP_TABLE)DUMMY)DUMMY)Every
DUMMYrecord in all three images is 292.6x — 16388 bytes on disk for 56 bytes of stream — and the singleDROP_TABLEis 75.9x, 16388 for 216. These clusters are idle and retain almost no real binlogs, so the absolute sizes are small; the ratios are the point.2. How large is a binlog record, by type. The 16KB floor only dominates if real records are well below it, so each type was measured with its own record class, the same
GsonUtils.GSONthatBinlogManager.addXxxRecorduses, and the realTBinlogthrift encoding (record bytes include the 4 byte length prefix):DUMMYDROP_TABLEUPSERTUPSERTUPSERTUPSERTUPSERTCREATE_TABLEADD_PARTITIONUPSERTis emitted once per committed transaction, so it dominates the record population of any actively ingesting cluster, andDUMMYis one per database and per table. For anUPSERTto reach 16KB a single transaction has to touch about 92 partitions (1 tablet each), or 24 partitions x 32 tablets, or 8 partitions x 128 tablets — roughly 500 to 1000 tablets in one commit. ACREATE_TABLEreaches 16KB at about 293 columns.ADD_PARTITIONis the exception and is worth stating plainly:AddPartitionRecordembeds the wholePartition->MaterializedIndex->Tablet->Replicatree, and crosses 16KB at about 28 buckets x 3 replicas, which is an ordinary table layout. Those records are still padded, just to the next doubled capacity rather than to 16KB, so the fix is worth ~2x for them instead of ~40x. They are also emitted once per partition creation rather than once per transaction.3. A/B on a realistic workload. Records written to a real file with
fsync, then read back through the releasedBinlogManager. JDK 17, one JVM per configuration. Image size and allocation are deterministic and came out byte-identical in every run; wall clock is given as the range over the runs, because the host is a shared and heavily loaded test machine.5000 table dummies + 20 db dummies + 100000 UPSERT binlogs (~300 B json each), 105020 records — 6 runs
20000 table dummies + 50 db dummies + 10000 UPSERT binlogs, 30050 records (dummy dominated) — 3 runs
Note the write-side allocation is unchanged (1.0x): this PR does not touch the
TMemoryBuffergrowth chain, only what gets written out of it. The read-side reduction comes fromreadTBinlogFromStreamno longer allocating padded arrays.4. Sensitivity to a partition-creation heavy workload. Starting from the measured run above (its 429 B average
UPSERTrecord) and mixing inADD_PARTITIONrecords of 32 buckets x 3 replicas:ADD_PARTITION(the measured run)The gain degrades gracefully with the share of large records and never turns negative.
The per-type and real-image numbers were produced against a released 4.0.5 binary, so the
ADD_PARTITIONrow reflects that version'sPartitionserialization; theUPSERTrecord layout is unchanged on master.Compatibility
The record framing is unchanged (
int length+ payload), noFeMetaVersionbump is needed, andwriteTBinlogToStreamhas a single caller — the image write path. The CCRget_binlogRPC path is not involved.Both directions were measured, not just argued, by feeding both formats to the
BinlogManagerof a released Doris 4.0.5 binary:TBinlog.read(); the read side is unchanged by this PR.Release note
None
Check List (For Author)
BinlogManagerTest.testWriteTBinlogWithActualLength: writes a small and a larger-than-BUFFER_SIZEbinlog, asserts that every record length in the stream equals the actual thrift serialized length rather than the buffer capacity, that the stream contains no trailing bytes, and that the binlogs still round trip).BinlogManagerTestis 7/7 green with this change. Reverting only the production change and keeping the test makes it fail withexpected: <52> but was: <16384>at the per-record length assertion, i.e. it is a real regression test for this bug.🤖 Generated with Claude Code