This change is 

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

pkg/storage/engine/mvcc.go, line 234 at r1 (raw file):

	var ms enginepb.MVCCStats

	if false {

Remove this before merging?

pkg/storage/engine/mvcc.go, line 255 at r1 (raw file):

	if isSysLocal(key) {
		// Handling system-local keys is straightforward because no ageable quantities

If there are transactions and intents (which we handle below), there are age-able quantities. Specifically, the range-local copy of the range descriptor has historical revisions that need to be GC'd. Maybe it's small enough to ignore that for the purposes of GCBytesAge, but the comment should indicate that that's what it's doing.

pkg/storage/engine/mvcc.go, line 413 at r1 (raw file):

	var ms enginepb.MVCCStats

	if false {

Either delete this or put in a comment about why this disabled code is here.

pkg/storage/engine/mvcc.go, line 446 at r1 (raw file):

	// Jump into the method below for extensive commentary on their semantics
	// and "rules one and two".
	_ = updateStatsOnPut

Nice hyperlink :)

pkg/storage/engine/mvcc.go, line 2666 at r1 (raw file):

	var prevKey []byte
	first := false
	var deletionNanos int64

s/deletionNanos/overwriteNanos/?

This could also use a comment about how we're iterating in reverse chronological order, so this tracks the timestamp of a value for use in computing the GC timestamp of its predecessor.

pkg/storage/engine/mvcc.go, line 2702 at r1 (raw file):

		if !isValue || implicitMeta {
			metaKeySize := int64(len(unsafeKey.Key)) + 1 // unsafeKey.EncodedSize()

What's up with this comment?

Comments from Reviewable

Reviewed 1 of 3 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

pkg/storage/engine/mvcc.go, line 227 at r1 (raw file):

func updateStatsOnPut(
	key roachpb.Key,
	prevKeySize, prevValSize int64,

It doesn't look like we need prevKeySize. In fact, it only looks like we need prevValSize to determine whether the previous value is a deletion tombstone. Should we trim these arguments? Perhaps just down to prevIsTombstone bool? That would also make a lot of the if prevValSize > 0 { checks easier to read.

pkg/storage/engine/mvcc.go, line 405 at r1 (raw file):

func updateStatsOnResolve(
	key roachpb.Key,
	prevKeySize, prevValSize int64,

Ditto

Comments from Reviewable

Review status: 0 of 4 files reviewed at latest revision, 8 unresolved discussions.

pkg/storage/engine/mvcc.go, line 227 at r1 (raw file):

We use prevValSize for arithmetic so I wasn't able to turn it into a bool, but we were not using prevKeySize and I removed it. I'm also using a local bool to simplify reading the logic, see if that makes more sense now.

pkg/storage/engine/mvcc.go, line 255 at r1 (raw file):

I clarified that we just don't track them but could.

pkg/storage/engine/mvcc.go, line 405 at r1 (raw file):

Done.

pkg/storage/engine/mvcc.go, line 413 at r1 (raw file):

Done, 🤞 that we won't need it any more.

pkg/storage/engine/mvcc.go, line 2666 at r1 (raw file):

Added comment. Renamed to accrueGCAgeNanos which I like because it specifies what we're using it for.

pkg/storage/engine/mvcc.go, line 2702 at r1 (raw file):

I toyed around with using EncodedSize() here instead, but decided it wasn't worth it. Removed.

Comments from Reviewable

Review status: 0 of 4 files reviewed at latest revision, 7 unresolved discussions.

pkg/storage/engine/mvcc.go, line 234 at r1 (raw file):

Done.

Comments from Reviewable

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

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

  // NB: making this uninitialized triggers compiler warnings
  // with `-Werror=maybe-uninitialized`. This warning seems like
  // a false positive (changing the above line to `first=false`

The above line is first=false, so maybe this comment meant something else?

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

          stats.gc_bytes_age += total_bytes * age_factor(wall_time, now_nanos);
        } else {
          stats.gc_bytes_age += total_bytes * age_factor(accrue_gc_age_nanos, now_nanos);

Maybe assert here that accrue_gc_age_nanos is non-zero to ensure we didn't hit the uninitialized path that the compiler was warning about.

Comments from Reviewable

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

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

Nope, just wanted to write true. Fixed.

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

Done.

Comments from Reviewable