bulkmerge: implement SST merging in bulk merge processor
Previously, the bulkMergeProcessor was a stub that returned placeholder output URIs without performing any actual SST merging. This made the distributed merge infrastructure non-functional for real workloads.
This commit implements the core merging logic in the processor by:
-
Adding
mergeSSTsmethod that identifies overlapping SSTs for each task's key range, creates an iterator over the external SST files, and writes merged output while respecting configurable size limits. -
Introducing a new
Mergefunction that creates and executes the DistSQL merge flow, collects results from all processors, and sorts the output SSTs by their start key for ingestion. -
Adding comprehensive tests for both single-node and multi-node distributed merge scenarios, including test infrastructure for creating and verifying SST merge results.
The implementation now produces correctly merged, non-overlapping SST files that can be ingested into ranges, making the distributed bulk merge functionality operational.
Fixes: #156658 Release note: None
Potential Bug(s) Detected
The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.
Next Steps: Please review the detailed findings in the workflow run.
Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.
After you review the findings, please tag the issue as follows:
- If the detected issue is real or was helpful in any way, please tag the issue with
O-AI-Review-Real-Issue-Found - If the detected issue was not helpful in any way, please tag the issue with
O-AI-Review-Not-Helpful
nit: is there a reason we pick 60mb as the default? wondering if it should be 64mb so that it's a multiple of 8.
I think this is to work around the max raft apply size. This is part of why I think we should use the sst_batcher to flush to kv in the final merge phase. It knows how to build, split, and retry sst writes.
@jeffswenson Do I need to set the default value of bulkio.merge.file_size back down to 60MB?
@jeffswenson Do I need to set the default value of bulkio.merge.file_size back down to 60MB?
It depends on how the ingestion phase works. If it tries to blindly write the sst to kv, it will need to be less than 64 MiB. The extra 4MiB was there for RPC overhead.
@jeffswenson Do I need to set the default value of bulkio.merge.file_size back down to 60MB?
It depends on how the ingestion phase works. If it tries to blindly write the sst to kv, it will need to be less than 64 MiB. The extra 4MiB was there for RPC overhead.
Okay, I set it back down to 60MB and added a comment. We can address this in a follow-on patch.
bors r+
bors retry
bors retry
bors retry
bors r+
Build succeeded:
Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.
Issue #156658: branch-release-26.1.
:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.
I noticed the backport to 26.1 was opened then closed (maybe because of a CLA issue??). Were we planning on back porting this eventually? I have a PR that I'd like to get in but is blocked because this one isn't in yet.