feat: add segmenting and wal persistence to WriteBuffer #24624
Conversation
pauldix
changed the title
There was a problem hiding this comment.
Hey @pauldix I had a question around using a separate thread vs a tokio task I'd like some clarity on before approving and I had a few suggestions for a few Rust based things. Overall though, I think these changes are great and all of the tests makes me feel more confident about them.
| pub fn new(segment_state: Arc<RwLock<SegmentState>>) -> Self { | ||
| let (shutdown_tx, shutdown_rx) = watch::channel(()); | ||
| let (buffer_tx, buffer_rx) = mpsc::channel(BUFFER_CHANNEL_LIMIT); | ||
| let (io_flush_tx, io_flush_rx) = std::sync::mpsc::channel(); |
There was a problem hiding this comment.
The MPSC type in the standard lib is kind of notorious for being slow. I'd recommend using crossbeam instead -> https://docs.rs/crossbeam/latest/crossbeam/channel/index.html
| async fn run_wal_op_buffer( | ||
| segment_state: Arc<RwLock<SegmentState>>, | ||
| mut buffer_rx: mpsc::Receiver<BufferedWrite>, | ||
| io_flush_tx: std::sync::mpsc::Sender<Vec<WalOp>>, |
There was a problem hiding this comment.
I think we can do an import at the top like:
use std::sync::mpsc::Sender as StdSender;
use std::sync::mpsc::Receiver as StdReceiver;letting you just write StdSender<Vec<WalOp>> and reducing the line noise.
| pub fn new(segment_state: Arc<RwLock<SegmentState>>) -> Self { | ||
| let (shutdown_tx, shutdown_rx) = watch::channel(()); | ||
| let (buffer_tx, buffer_rx) = mpsc::channel(BUFFER_CHANNEL_LIMIT); | ||
| let (io_flush_tx, io_flush_rx) = std::sync::mpsc::channel(); |
There was a problem hiding this comment.
You can alias the import with use std::sync::mpsc as std_mpsc so that you can just do std_mpsc::channel() and keeping things a bit consistent and less verbose looking.
| { | ||
| Some(writer) => writer, | ||
| None => Box::new(WalSegmentWriterNoopImpl::new(next_segment_id)), | ||
| }; |
There was a problem hiding this comment.
You can replace this with the unwrap_or_else function and it gets rid of the need for a match.
| { | |
| Some(writer) => writer, | |
| None => Box::new(WalSegmentWriterNoopImpl::new(next_segment_id)), | |
| }; | |
| .unwrap_or_else(|| Box::new(WalSegmentWriterNoopImpl::new(next_segment_id))); |
pauldix
force-pushed
the
pd/write-buffer-wal-gitfix
branch
2 times, most recently
from
fe15283 to
3cce1fd
Compare
There was a problem hiding this comment.
@pauldix looks good to me. 😄 My concerns were addressed. I think before merging, you should rebase off main or merge it in to run tests since my PR for different outputs is in there now. I don't think it should cause issues, but I would hate to have a skew merge issue where the code will merge but the tests fail.
This creates the WriteBufferFlusher and OpenBufferSegment. If a wal is passed into the buffer, data written into it will be persisted to the wal for the initialized segment id.
pauldix
force-pushed
the
pd/write-buffer-wal-gitfix
branch
from
3cce1fd to
83f7ed3
Compare
|
Rebased from main, will merge on green |
* refactor: move write buffer into its own dir * feat: implement write buffer segment with wal flushing This creates the WriteBufferFlusher and OpenBufferSegment. If a wal is passed into the buffer, data written into it will be persisted to the wal for the initialized segment id. * refactor: use crossbeam in flusher and pr cleanup
This moves the write buffer into its own directory in the crate. It puts writes into a segment, which are persisted into the WAL if it is configured. Writes are buffered up before committed to ensure we don't do file IO with every individual write.
This doesn't yet load data from the wal into the buffer, which will come in a follow on PR. The bootup routine will also need to use the persister to know which wal segments should be loaded into the buffer. Finally, the buffer will need to rotate and persist segments.
Fixes #24571