db: remove compaction/flush pacing

[jbowens]

@nvanbenschoten uncovered unnecessary compaction-loop contention over in #1808. Since the flush/compaction pacing code has been disabled for 2+ years, I think it's time to rip it out altogether so we're not maintaining dead code.

---

db: remove compaction.atomicBytesIterated

The atomic bytes-iterated value was used for compaction and flush pacing.
Remove it for now to resolve #1808.

Fix #1808.

db: remove compaction/flush pacing

Compaction and flush pacing have been disabled for several years. Remove the
code to avoid maintaining dead code. If/when we attempt to re-enable pacing, we
can revert this commit.

Informs #687.

[jbowens]

@sumeerbhola Is the InternalIntervalMetrics.Flush.WriteThroughput indeed intended to measure the bytes iterated over in the flushables instead of the bytes written to disk? I had assumed the bytesFlushed return value was the latter and was surprised by this.

[cockroach-teamcity]

This change is 

[nicktrav]

Reviewed 7 of 7 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jbowens and @nicktrav)

---

options.go line 1154 at r3 (raw file):

			case "mem_table_stop_writes_threshold":
				o.MemTableStopWritesThreshold, err = strconv.Atoi(value)
			case "min_compaction_rate":

My understanding is that one can set these manually as options for a Cockroach store. If someone was passing them in, even if they were doing nothing before, do we continue to support them, vs. returning an error (either here, or in the default arm)? Wary of the "may be meaningful again" implying cruft we carry around.

No strong feeling either way.

---

pacer.go line 29 at r3 (raw file):

}

// compactionInternalPacer contains fields and methods common to compactionPacer

Noice. 📉

[jbowens]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nicktrav)

---

options.go line 1154 at r3 (raw file):

Previously, nicktrav (Nick Travers) wrote…

My understanding is that one can set these manually as options for a Cockroach store. If someone was passing them in, even if they were doing nothing before, do we continue to support them, vs. returning an error (either here, or in the default arm)? Wary of the "may be meaningful again" implying cruft we carry around.

No strong feeling either way.

These options had defaults, so they should appear in all Cockroach stores' OPTIONS files. We need to keep them within this switch statement, because Pebble will error when it observes unrecognized options.

pebble/options.go

Lines 1080 to 1083 in d3484a6

	// WARNING: DO NOT remove entries from the switches below because doing so
	// causes a key previously written to the OPTIONS file to be considered unknown,
	// a backwards incompatible change. Instead, leave in support for parsing the
	// key but simply don't parse the value.

[nicktrav]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nvanbenschoten)

---

options.go line 1154 at r3 (raw file):

Previously, jbowens (Jackson Owens) wrote…

These options had defaults, so they should appear in all Cockroach stores' OPTIONS files. We need to keep them within this switch statement, because Pebble will error when it observes unrecognized options.

pebble/options.go

Lines 1080 to 1083 in d3484a6

	// WARNING: DO NOT remove entries from the switches below because doing so
	// causes a key previously written to the OPTIONS file to be considered unknown,
	// a backwards incompatible change. Instead, leave in support for parsing the
	// key but simply don't parse the value.

Ah nice. Thanks.

[jbowens]

TFTR!

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nvanbenschoten)

[sumeerbhola]

Very glad to see this cleanup.

Reviewable status: all files reviewed, 1 unresolved discussion

---

compaction.go line 1610 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

@sumeerbhola Is the InternalIntervalMetrics.Flush.WriteThroughput indeed intended to measure the bytes iterated over in the flushables instead of the bytes written to disk? I had assumed the bytesFlushed return value was the latter and was surprised by this.

It is slightly messy either way.

With bytes written to disk, the byte tokens computed based on compactions out of L0 and flushes into L0 are comparable.
But when we look at the size of a pebble batch as a basis for estimating how many tokens, that is uncompressed (this doesn't matter currently since everything is estimated based on the compressed bytes). Also, it is possible for many of the memtable bytes to get deleted (due to raft log truncations).

I think the former is probably better, as you are indicating.

[jbowens]

Reviewable status: all files reviewed, 1 unresolved discussion

---

compaction.go line 1610 at r2 (raw file):

Previously, sumeerbhola wrote…

It is slightly messy either way.

With bytes written to disk, the byte tokens computed based on compactions out of L0 and flushes into L0 are comparable.
But when we look at the size of a pebble batch as a basis for estimating how many tokens, that is uncompressed (this doesn't matter currently since everything is estimated based on the compressed bytes). Also, it is possible for many of the memtable bytes to get deleted (due to raft log truncations).

I think the former is probably better, as you are indicating.

That makes sense.

I was asking partially because if we switch it to use the bytes written to disk, I think we'd remove the last use of the bytes-iterated calculation. We could remove some more code and clean up some internal interfaces.