Both the KVs and Bytes response formats go through the same mvccScanner/pebbleMVCCScanner code paths. We always scan to the Bytes format, and then convert to KVs if necessary. I think you only have to do this work twice, not four times.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal and @tbg)

pkg/storage/engine/pebble_mvcc_scanner.go, line 536 at r1 (raw file):

			// pretend we reached maxKeys. (The resume key computation requires
			// fetching an additional key after that, so trying to exit the
			// iterations based on size alone is not trivial).

That is a bit hacky, though it seems safer than trying to extend the resume key logic.

though @itsbilal should give this a review as well.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @itsbilal and @tbg)

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

  put(value.data(), value.size(), 0);
  count_++;
  bytes_ += 8 + key.size() + value.size(); // see (*pebbleResults).put

s/8/sizeof(size_buf)/g

pkg/storage/engine/testdata/mvcc_histories/target_bytes, line 1 at r2 (raw file):

Reverse scans?

Perhaps a test where keys are skipped. That is, show that the target bytes is only affected by keys that are added to the result, not keys that are skipped because they are too new.

pkg/storage/engine/testdata/mvcc_histories/target_bytes, line 7 at r2 (raw file):

  put      k=a v=abcdef
  put      k=c v=ghijkl
  put      k=e v=mnopqr

Might want to vary the value size. Right now, every key and value has the same size. This does make the results line up nicely, but is perhaps a bit too uniform for testing purposes.

thanks!

Reviewed 10 of 10 files at r2.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @tbg)

c-deps/libroach/mvcc.h, line 728 at r2 (raw file):

  const rocksdb::Slice end_key_;
  int64_t max_keys_;
  const int64_t target_bytes_; // see pebbleMVCCScanner

Might be worth saying see MVCCScanOptions instead, since the big comment explaining this is there?

pkg/storage/engine/mvcc_history_test.go, line 700 at r2 (raw file):

		max = int64(imax)
	}
	if key := "tb"; e.hasArg(key) {

This works but I'm wondering if we can use a more descriptive arg name for this, given my first instinct was "tb? sounds like TomBstones". targetbytes probably works better and is less ambiguous.

pkg/storage/engine/pebble_mvcc_scanner.go, line 539 at r2 (raw file):

			// number of keys.
			//
			// TODO(bilal): see if this can be implemented more transparently.

👍 on the todo

pkg/storage/engine/testdata/mvcc_histories/target_bytes, line 1 at r2 (raw file):

👍 on the reverse scan suggestion.

Also, another possibility is zero-sized values (basically tombstones) and scans with tombstones=true

TFTRs! I souped up the tests with reverse iteration and tombstones (and the combination of both). Everything worked as expected.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @petermattis)

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

Done.

pkg/storage/engine/testdata/mvcc_histories/target_bytes, line 7 at r2 (raw file):

Done.