storage, ccl: Use RocksDBv2 format for SSTs meant for ingestion

[itsbilal]

This change updates SSTWriter to use the RocksDBv2 format and to
re-enable bloom filters on all SSTs written for ingestion (eg.
snapshots). SSTs written for backup are still written in the
leveldb format, and TestPebbleWritesSameSSTables still uses
the leveldb format for comparison with those produced by
RocksDBSstFileWriter.

Fixes #41814.

Release note: None.

[cockroach-teamcity]

This change is 

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @dt, @itsbilal, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/sst_writer.go, line 39 at r1 (raw file):

// is being written for ingestion, and when false, bloom filters are disabled
// and the SST format is changed to LevelDB.
func MakeSSTWriter(f writeCloseSyncer, ingestion bool) SSTWriter {

Passing a boolean ingestion argument makes it difficult to read what is going on at the caller. You can improve the situation by adding an inline comment on that parameter, but in this case the parameter is (almost) always a constant and there is a better option: provide separate methods. Something like:

MakeBackupSSTWriter(...) {
}

MakeIngestionSSTWriter(...) {
}

[itsbilal]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @dt, @petermattis, and @sumeerbhola)

---

pkg/storage/engine/sst_writer.go, line 39 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Passing a boolean ingestion argument makes it difficult to read what is going on at the caller. You can improve the situation by adding an inline comment on that parameter, but in this case the parameter is (almost) always a constant and there is a better option: provide separate methods. Something like:

MakeBackupSSTWriter(...) {
}

MakeIngestionSSTWriter(...) {
}

That's more readable, yes. Thanks! Done.

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajkr, @dt, @itsbilal, and @sumeerbhola)

---

pkg/storage/engine/sst_writer_test.go, line 112 at r2 (raw file):

	kvs := makeIntTableKVs(numKeys, valueSize, revisions)
	sstRocks := makeRocksSST(t, kvs)
	sstPebble := makePebbleSST(t, kvs, false)

Nit: add an inline comment here: false /* ingestion */.

---

pkg/storage/engine/sst_writer_test.go, line 174 at r2 (raw file):

	b.StartTimer()
	for i := 0; i < ssts; i++ {
		_ = makePebbleSST(b, kvs, true)

Nit: add an inline comment here: true /* ingestion */.

[itsbilal]

Thanks for the reviews!

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[itsbilal]

TestCCLLogic flake.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[itsbilal]

Different unrelated compile time “no such file or directory” error.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[itsbilal]

Yet another unrelated failure, this time in TestChangefeedTelemetry. My luck is hilariously bad today. The union of the above test runs is still a pass.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)