Rate Limit ScanStatistics

[raggar]

This pull request uses a TokenBucket to limit the number of keys that
read from ScanStatistics within a certain period of time.

Changes rely on #2713

Fixes: #2778

[cockroach-teamcity]

This change is 

[raggar]

Not sure what values to put here (I just put placeholders for now)

[RaduBerinde]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @jbowens and @RahulAggarwal1016)

---

db.go line 2605 at r3 (raw file):

// ScanStatistics returns the count of different key kinds within the lsm for a
// key span [lower, upper) as well as the number of snapshot keys.
func (d *DB) ScanStatistics(ctx context.Context, lower, upper []byte) (LSMKeyStatistics, error) {

Perhaps in a separate change, it would be nice if we could make this stop if the context is cancelled
Not sure if we want ScanInternal to do that in general (CC @itsbilal), if not we can just try to poll ctx.Done() in each callback.

That would make it possible to interrupt the query and stop the scan, which would be useful if we accidentally passed in a very big key range.

---

db.go line 2609 at r3 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

Not sure what values to put here (I just put placeholders for now)

@jbowens maybe has an idea of how many keys per second would not be a problem for most deployments. If I had to guess, on the order of 100K to 1M keys per second?

---

db.go line 2612 at r3 (raw file):

	rateLimit := func() {
		fulfilled, tryAgainAfter := tb.TryToFulfill(1)

This needs to be done in a loop

[jbowens]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @jbowens, @RaduBerinde, and @RahulAggarwal1016)

---

db.go line 2609 at r3 (raw file):

Previously, RaduBerinde wrote…

@jbowens maybe has an idea of how many keys per second would not be a problem for most deployments. If I had to guess, on the order of 100K to 1M keys per second?

I think we should rate limit on InternalKey.Size()+lazyValue.Len() so that we limit the read bandwidth. Also, I think the rate should be passed in by the user (and maybe even through the SQL builtin).

[raggar]

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @jbowens and @RaduBerinde)

---

db.go line 2609 at r3 (raw file):

Previously, jbowens (Jackson Owens) wrote…

I think we should rate limit on InternalKey.Size()+lazyValue.Len() so that we limit the read bandwidth. Also, I think the rate should be passed in by the user (and maybe even through the SQL builtin).

for limiting based on size, what should a reasonable value for the rate and burst be (to also use in tests)?

[RaduBerinde]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @jbowens and @RahulAggarwal1016)

---

db.go line 2678 at r5 (raw file):

type ScanStatisticsOptions struct {
	rateLimitEnabled bool
	rate             float64

I'd change to LimitBytesPerSecond int64. 0 could mean no limit.

---

db.go line 2690 at r5 (raw file):

	var prevKey InternalKey
	tb := tokenbucket.TokenBucket{}
	tb.Init(tokenbucket.TokensPerSecond(opts.rate), tokenbucket.Tokens(opts.burst))

I don't think "burst" needs to be configurable, we can just hardcode it to something like 1024 bytes.

---

db.go line 2690 at r5 (raw file):

	var prevKey InternalKey
	tb := tokenbucket.TokenBucket{}
	tb.Init(tokenbucket.TokensPerSecond(opts.rate), tokenbucket.Tokens(opts.burst))

Add a comment explaining that each "token" (roughly) corresponds to a byte that was read.

[raggar]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @jbowens and @RaduBerinde)

---

db.go line 2612 at r3 (raw file):

Previously, RaduBerinde wrote…

This needs to be done in a loop

fixed!

---

db.go line 2678 at r5 (raw file):

Previously, RaduBerinde wrote…

I'd change to LimitBytesPerSecond int64. 0 could mean no limit.

fixed!

---

db.go line 2690 at r5 (raw file):

Previously, RaduBerinde wrote…

I don't think "burst" needs to be configurable, we can just hardcode it to something like 1024 bytes.

fixed!

---

db.go line 2690 at r5 (raw file):

Previously, RaduBerinde wrote…

Add a comment explaining that each "token" (roughly) corresponds to a byte that was read.

fixed!

[raggar]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)

---

db.go line 2605 at r3 (raw file):

Previously, RaduBerinde wrote…

Perhaps in a separate change, it would be nice if we could make this stop if the context is cancelled
Not sure if we want ScanInternal to do that in general (CC @itsbilal), if not we can just try to poll ctx.Done() in each callback.

That would make it possible to interrupt the query and stop the scan, which would be useful if we accidentally passed in a very big key range.

yeah this could be interesting, would it be like we cancel the context after we have read a certain number of bytes? Also, should I make a follow up issue for this if it is something that we want?

[RaduBerinde]

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @itsbilal, @jbowens, and @RahulAggarwal1016)

---

db.go line 2605 at r3 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

yeah this could be interesting, would it be like we cancel the context after we have read a certain number of bytes? Also, should I make a follow up issue for this if it is something that we want?

If you run a SQL statement in the sql shell I believe you can interrupt it and that should cancel the relevant contexts. That's the use case I have in mind.

Sure, you can file an issue. Once https://github.com/cockroachdb/tokenbucket/pull/1/files merges, we can replace the loop with WaitCtx

---

db.go line 2679 at r9 (raw file):

type ScanStatisticsOptions struct {
	// LimitBytesPerSecond indicates the number of bytes that are able to be read
	// per second using ScanInternal.

[nit] Mention that 0 means no limit.

[raggar]

Reviewable status: 0 of 7 files reviewed, 7 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)

---

db.go line 2679 at r9 (raw file):

Previously, RaduBerinde wrote…

[nit] Mention that 0 means no limit.

fixed!

[raggar]

hm right now I am not using this BytesRead field, did you have an idea for if we should use it for rate limiting @jbowens

[jbowens]

Reviewed 2 of 6 files at r8, 2 of 4 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: 5 of 7 files reviewed, 8 unresolved discussions (waiting on @itsbilal, @RaduBerinde, and @RahulAggarwal1016)

---

db.go line 2675 at r10 (raw file):

Previously, RahulAggarwal1016 (Rahul Aggarwal) wrote…

hm right now I am not using this BytesRead field, did you have an idea for if we should use it for rate limiting @jbowens

Let's still surface it in the output, even if we don't need it for rate limiting because it's useful information in its own right. We should add a comment describing what it is (logical, pre-compression size of keys and values read).