storage: drop raftentry.Cache data in applySnapshot

[ajwerner]

This PR adds a new .Drop method to the raftentry.Cache which will clear all data associated with a range more efficiently than calling .Clear with a large index. The second commit then uses this call when applying a snapshot to ensure that stale cached raft entries are never used.

Fixes #37056.

[cockroach-teamcity]

This change is 

[ajwerner]

This PR could benefit from a storage test

[ajwerner]

storage unit test coming in a separate PR courtesy of @nvanbenschoten.

[nvanbenschoten]

mod two nits. Thanks for getting this up so quickly.

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)

---

pkg/storage/replica_raftstorage.go, line 861 at r2 (raw file):

	// Clear the cached raft log entries to ensure that old or uncommitted
	// entries don't impact the in-memory state.
	r.store.raftEntryCache.Drop(r.RangeID)

You might consider adding a TODO to the method to use it in more places.

---

pkg/storage/raftentry/cache_test.go, line 456 at r1 (raw file):

	var wg sync.WaitGroup
	// Test using both Clear and Drop to remove the added entries.
	testutils.RunTrueAndFalse(t, "dropOrClear", func(t *testing.T, useClear bool) {

nit: this isn't a great use of RunTrueAndFalse because it will be printed as:

TestConcurrentUpdates/dropOrClear=false
TestConcurrentUpdates/dropOrClear=true

It's not really clear what either sub-test does. Maybe consider:

for _, t := range struct{
    name string
    clear func()
}{
    {"drop", func() { c.Drop(1) }},
    {"clear", func() { c.Clear(1, 22) }},
}{
    ...

[ajwerner]

Thanks for the quick review and for writing the test!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/storage/replica_raftstorage.go, line 861 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You might consider adding a TODO to the method to use it in more places.

Anywhere in particular come to mind? A quick glance at the uses of Clear didn't reveal much to me in terms of replacement outside of some tests. One place we could use it is in replica_destroy. Where would the TODO go? Here or on the method? Either one feels like pollution without something specific in mind. If the thought was just about replica gc, I can just type a PR instead of a TODO.

---

pkg/storage/raftentry/cache_test.go, line 456 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: this isn't a great use of RunTrueAndFalse because it will be printed as:

TestConcurrentUpdates/dropOrClear=false
TestConcurrentUpdates/dropOrClear=true

It's not really clear what either sub-test does. Maybe consider:

for _, t := range struct{
    name string
    clear func()
}{
    {"drop", func() { c.Drop(1) }},
    {"clear", func() { c.Clear(1, 22) }},
}{
    ...

Done

[ajwerner]

@nvanbenschoten
Is this where you had in mind for the additional Drop call?

cockroach/pkg/storage/replica_destroy.go

Lines 77 to 81 in 8c4ba4b

	err := clearRangeData(ctx, desc, reader, batch, destroyData)
	if err != nil {
	return err
	}

[nvanbenschoten]

Is this where you had in mind for the additional Drop call?

I was thinking one level above that, in Replica.destroyRaftMuLocked. After the batch is actually committed.

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

[nvanbenschoten]

No need to make that change now though. We'll want this to be small enough to backport.

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

[ajwerner]

bors r=nvanbenschoten

[craig]

Build succeeded

• GitHub CI (Cockroach)