Sorry John, I didn't mean to mis-characterize it like that. I was mostly referring to disabling memtables. AFAIK the SstFileWriter API is primarily designed for bulk ingest, e.g. for bootstrapping a database from a backup, rather than during normal operation of an online database. That said, I was overly alarmist in my phrasing.
My concern is only that, while the concept seems quite reasonable, there are no doubt hidden issues lurking. On Wed, 21 Jun 2023 at 18:25, John Roesler <vvcep...@apache.org> wrote: > Thanks Nick, > > That sounds good to me. > > I can't let (2) slide, though.. Writing and ingesting SST files is not a > RocksDB internal, but rather a supported usage pattern on public APIs. > Regardless, I think your overall preference is fine with me, especially > if we can internalize this change within the store implementation itself. > > Thanks, > -John > > On 6/21/23 11:50, Nick Telford wrote: > > Hi Bruno, > > > > 1. > > Isn't this exactly the same issue that WriteBatchWithIndex transactions > > have, whereby exceeding (or likely to exceed) configured memory needs to > > trigger an early commit? > > > > 2. > > This is one of my big concerns. Ultimately, any approach based on > cracking > > open RocksDB internals and using it in ways it's not really designed for > is > > likely to have some unforseen performance or consistency issues. > > > > 3. > > What's your motivation for removing these early commits? While not > ideal, I > > think they're a decent compromise to ensure consistency whilst > maintaining > > good and predictable performance. > > All 3 of your suggested ideas seem *very* complicated, and might actually > > make behaviour less predictable for users as a consequence. > > > > I'm a bit concerned that the scope of this KIP is growing a bit out of > > control. While it's good to discuss ideas for future improvements, I > think > > it's important to narrow the scope down to a design that achieves the > most > > pressing objectives (constant sized restorations during dirty > > close/unexpected errors). Any design that this KIP produces can > ultimately > > be changed in the future, especially if the bulk of it is internal > > behaviour. > > > > I'm going to spend some time next week trying to re-work the original > > WriteBatchWithIndex design to remove the newTransaction() method, such > that > > it's just an implementation detail of RocksDBStore. That way, if we want > to > > replace WBWI with something in the future, like the SST file management > > outlined by John, then we can do so with little/no API changes. > > > > Regards, > > > > Nick > > >
