@itsbilal @sumeerbhola thoughts on this approach?

This is motivated largely by the pragmatic concern that all our test cases assume that once a sstable is larger than the target size, we'll split. If we start to prevent user keys from being split across sstables, we can't split immediately: we need to wait until we also observe a user key change (because we don't buffer the user key until we want to split).

Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @itsbilal and @sumeerbhola)

I looked mainly at the implementation, and it made sense. Wasn't sure if there was some subtle background that I was missing though.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @itsbilal and @sumeerbhola)

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jbowens and @sumeerbhola)

compaction.go, line 326 at r1 (raw file):

// any splitters that don't guarantee user key splits (i.e. splitters that make
// their determination in ways other than comparing the current key against a
// limit key.) If a wrapped splitter advises a split, it must continue

Should this be moved up to the splitter interface comment as well? I'm good with making it a part of the splitter contract, so to speak - if shouldSplitBefore(a) advises a split then shouldSplitBefore(b) should also advise a split given b >= a, until onNewOutput is called.

TFTRs!

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @itsbilal, @nicktrav, and @sumeerbhola)

compaction.go, line 326 at r1 (raw file):

Good call, done.

Reviewed 2 of 3 files at r2.
Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions

-- commits, line 27 at r2:

I didn't understand this comment, specifically about future commits. Hasn't this code already changed userKeyChangeSplitter in this manner?

compaction.go, line 333 at r2 (raw file):

	cmp               Compare
	splitter          compactionOutputSplitter
	unsafePrevUserKey func() []byte

apologies for the extreme tardiness in reviewing this code.
I really like the elimination of this key copy.

compaction.go, line 2404 at r2 (raw file):

			},
		}
		outputSplitters = append(outputSplitters, &l0LimitSplitter{c: c, ve: ve})

Can you remind me why having these two splitters being peers in a splitterGroup is correct, in that the group does not split a userkey across two sstables?
Is it because we know that the l0LimitSplitter also splits on a user key boundary because it uses the limit?

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions

-- commits, line 27 at r2:

Sorry, this was poorly worded. This is referring to this case: 1. In thinking about this again, this case will become irrelevant if we start using userKeyChangeSplitter everywhere.

compaction.go, line 333 at r2 (raw file):

🎊

compaction.go, line 2404 at r2 (raw file):

Yeah, exactly — it never requests splits mid-userkey, since it requests a split as soon as we've exceeded limit.