libroach: further decomposition of db.cc

[petermattis]

Split the various engine implementations out of db.cc. Move the merge
operator routines into merge.cc. Move the MVCC* routines into
mvcc.cc. Move the custom RocksDB comparator to comparator.cc.

This is all code movement: no functional changes.

Release note: None

[cockroach-teamcity]

This change is 

[petermattis]

I think this gets us to a reasonable place.

[petermattis]

Added a few others as reviewers who might have opinions about the organization of code in libroach.

[petermattis]

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

c-deps/libroach/ccl/db.cc, line 18 at r2 (raw file):

#include <rocksdb/write_batch.h>
#include "../batch.h"
#include "../comparator.h"

@benesch Do you know how to avoid these relative includes? Seems like there should be some cmake configuration.

---

Comments from Reviewable

[mberhault]

I'm overall happy with this kind of re-org, just really not while #21580 is pending, this will be a nasty merge.

---

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

c-deps/libroach/ccl/db.cc, line 18 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

@benesch Do you know how to avoid these relative includes? Seems like there should be some cmake configuration.

Things used in common by ./ and ccl/ should ideally be in sub-directories that are added to the include list. Easy enough for things like ../fmt.h, not so much for the ones listed here. The dirty way would be to include ./ as well.

---

Comments from Reviewable

[petermattis]

I'm overall happy with this kind of re-org, just really not while #21580 is pending, this will be a nasty merge.

I'll hold off on merging until #21580 goes in.

---

Review status: 0 of 26 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[tbg]

---

Reviewed 24 of 24 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

c-deps/libroach/batch.cc, line 33 at r1 (raw file):

// pointing to "key" if no updates existing for that key in the batch.
//
// Note that RocksDB WriteBatches append updates

nit: weird flow

---

c-deps/libroach/mvcc.cc, line 80 at r1 (raw file):

  std::string prev_key;
  bool first = false;
  // NB: making this uninitialized triggers compiler warnings

This is random, but you didn't have any opinions on this, did you? This would be the first time any compiler was wrong about any warning issued to me.

---

Comments from Reviewable

[mberhault]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

c-deps/libroach/ccl/db.cc, line 18 at r2 (raw file):

Previously, mberhault (marc) wrote…

Things used in common by ./ and ccl/ should ideally be in sub-directories that are added to the include list. Easy enough for things like ../fmt.h, not so much for the ones listed here. The dirty way would be to include ./ as well.

After checking, rocksdb does include its own PROJECT_SOURCE_DIR so that their includes look like util/blahblah.h. This seems reasonable to do for us as well. Moving things into separate subdirs can be done at a later date.

---

Comments from Reviewable

[bdarnell]

I think this may have gone a little too far towards lots of small files, but i don't have an alternative arrangement to offer.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[petermattis]

Review status: 23 of 26 files reviewed at latest revision, 3 unresolved discussions.

---

c-deps/libroach/batch.cc, line 33 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

nit: weird flow

Fixed.

---

c-deps/libroach/mvcc.cc, line 80 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

This is random, but you didn't have any opinions on this, did you? This would be the first time any compiler was wrong about any warning issued to me.

Seems reasonable to me that the compiler can't figure out that accrue_gc_age_nanos is being initialized before use. The compiler is doing some data flow analysis and you're running into the limits. This is fairly typical for C++ compiler warnings in my experience. We don't see this in Go because all variables are default 0 initialized.

---

Comments from Reviewable

[benesch]

Reviewed 23 of 24 files at r1, 3 of 3 files at r3, 1 of 2 files at r4.
Review status: 25 of 26 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

c-deps/libroach/options.h, line 29 at r3 (raw file):

// local variables:
// mode: c++
// end:

Stray editor-specific config.

---

c-deps/libroach/ccl/db.cc, line 18 at r2 (raw file):

Previously, mberhault (marc) wrote…

After checking, rocksdb does include its own PROJECT_SOURCE_DIR so that their includes look like util/blahblah.h. This seems reasonable to do for us as well. Moving things into separate subdirs can be done at a later date.

I'd hesitate to use RocksDB's build system as an example of what to do in general, but agreed, sticking PRIVATE . into libroachccl's target_include_directories seems reasonable.

(The relative imports don't bother me though.)

---

Comments from Reviewable

[petermattis]

Per in-person discussion with @mberhault, I'm going to merge this and will help with any conflicts that arise with his PR.

[petermattis]

Review status: 15 of 30 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

c-deps/libroach/options.h, line 29 at r3 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Stray editor-specific config.

Ack. I've removed all of these.

---

Comments from Reviewable