storage: drop raftentry.Cache data in applySnapshot by ajwerner · Pull Request #37055 · cockroachdb/cockroach

ajwerner · 2019-04-23T21:33:19Z

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 · 2019-04-23T21:33:29Z

This change is

ajwerner · 2019-04-23T21:33:29Z

This PR could benefit from a storage test

ajwerner · 2019-04-23T22:39:10Z

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

nvb

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) }},
}{
    ...

Release note: None

Add logic to clear cached raft log entries when applying a snapshot. Release note: None

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 · 2019-04-24T00:03:25Z

@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 
        
           }

nvb

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)

nvb

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 · 2019-04-24T00:23:41Z

bors r=nvanbenschoten

37055: storage: drop raftentry.Cache data in applySnapshot r=nvanbenschoten a=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. Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>

craig · 2019-04-24T00:42:46Z

Build succeeded

GitHub CI (Cockroach)

This PR adds a regression test for cockroachdb#37056. In doing so, it also confirms the theory that cockroachdb#37055 is the proper fix for that bug. Before cockroachdb#37055, the test would get stuck with the following logs repeatedly firing: ``` I190424 00:15:52.338808 12 storage/client_test.go:1242 SucceedsSoon: expected [21 21 21], got [12 21 21] I190424 00:15:52.378060 78 vendor/go.etcd.io/etcd/raft/raft.go:1317 [s1,r1/1:/M{in-ax}] 1 [logterm: 6, index: 31] rejected msgApp [logterm: 8, index: 31] from 2 I190424 00:15:52.378248 184 vendor/go.etcd.io/etcd/raft/raft.go:1065 [s2,r1/2:/M{in-ax}] 2 received msgApp rejection(lastindex: 31) from 1 for index 31 I190424 00:15:52.378275 184 vendor/go.etcd.io/etcd/raft/raft.go:1068 [s2,r1/2:/M{in-ax}] 2 decreased progress of 1 to [next = 31, match = 31, state = ProgressStateProbe, waiting = false, pendingSnapshot = 0] ``` After cockroachdb#37055, the test passes. Release note: None

37058: storage: create new TestSnapshotAfterTruncationWithUncommittedTail test r=nvanbenschoten a=nvanbenschoten This PR adds a regression test for #37056. In doing so, it also confirms the theory that #37055 is the proper fix for that bug. Before #37055, the test would get stuck with the following logs repeatedly firing: ``` I190424 00:15:52.338808 12 storage/client_test.go:1242 SucceedsSoon: expected [21 21 21], got [12 21 21] I190424 00:15:52.378060 78 vendor/go.etcd.io/etcd/raft/raft.go:1317 [s1,r1/1:/M{in-ax}] 1 [logterm: 6, index: 31] rejected msgApp [logterm: 8, index: 31] from 2 I190424 00:15:52.378248 184 vendor/go.etcd.io/etcd/raft/raft.go:1065 [s2,r1/2:/M{in-ax}] 2 received msgApp rejection(lastindex: 31) from 1 for index 31 I190424 00:15:52.378275 184 vendor/go.etcd.io/etcd/raft/raft.go:1068 [s2,r1/2:/M{in-ax}] 2 decreased progress of 1 to [next = 31, match = 31, state = ProgressStateProbe, waiting = false, pendingSnapshot = 0] ``` After #37055, the test passes. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

This PR adds a regression test for cockroachdb#37056. In doing so, it also confirms the theory that cockroachdb#37055 is the proper fix for that bug. Before cockroachdb#37055, the test would get stuck with the following logs repeatedly firing: ``` I190424 00:15:52.338808 12 storage/client_test.go:1242 SucceedsSoon: expected [21 21 21], got [12 21 21] I190424 00:15:52.378060 78 vendor/go.etcd.io/etcd/raft/raft.go:1317 [s1,r1/1:/M{in-ax}] 1 [logterm: 6, index: 31] rejected msgApp [logterm: 8, index: 31] from 2 I190424 00:15:52.378248 184 vendor/go.etcd.io/etcd/raft/raft.go:1065 [s2,r1/2:/M{in-ax}] 2 received msgApp rejection(lastindex: 31) from 1 for index 31 I190424 00:15:52.378275 184 vendor/go.etcd.io/etcd/raft/raft.go:1068 [s2,r1/2:/M{in-ax}] 2 decreased progress of 1 to [next = 31, match = 31, state = ProgressStateProbe, waiting = false, pendingSnapshot = 0] ``` After cockroachdb#37055, the test passes. Release note: None

ajwerner requested review from a team and nvb April 23, 2019 21:33

ajwerner force-pushed the ajwerner/raft-entry-cache-drop branch from a6fa12e to e917cc3 Compare April 23, 2019 22:25

ajwerner force-pushed the ajwerner/raft-entry-cache-drop branch 3 times, most recently from 083666b to 7c7d0a9 Compare April 23, 2019 23:17

ajwerner requested review from a team April 23, 2019 23:17

ajwerner requested a review from a team as a code owner April 23, 2019 23:17

ajwerner requested review from a team April 23, 2019 23:17

ajwerner force-pushed the ajwerner/raft-entry-cache-drop branch 2 times, most recently from 5546e70 to 9a6b4fa Compare April 23, 2019 23:21

ajwerner removed request for a team April 23, 2019 23:22

nvb approved these changes Apr 23, 2019

View reviewed changes

Andrew Werner added 2 commits April 23, 2019 19:55

raftentry: add Drop method to cache to efficiently clear a range

99ba14d

Release note: None

storage: drop raftentry.Cache data in applySnapshot

f8de052

Add logic to clear cached raft log entries when applying a snapshot. Release note: None

ajwerner force-pushed the ajwerner/raft-entry-cache-drop branch from 9a6b4fa to f8de052 Compare April 23, 2019 23:55

ajwerner commented Apr 23, 2019

View reviewed changes

nvb reviewed Apr 24, 2019

View reviewed changes

craig bot merged commit f8de052 into cockroachdb:master Apr 24, 2019

nvb mentioned this pull request Apr 24, 2019

storage: create new TestSnapshotAfterTruncationWithUncommittedTail test #37058

Merged

ajwerner mentioned this pull request Apr 24, 2019

release-19.1: storage: drop raftentry.Cache data in applySnapshot #37064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

storage: drop raftentry.Cache data in applySnapshot#37055

storage: drop raftentry.Cache data in applySnapshot#37055
craig[bot] merged 2 commits intocockroachdb:masterfrom
ajwerner:ajwerner/raft-entry-cache-drop

ajwerner commented Apr 23, 2019 •

edited

Loading

Uh oh!

cockroach-teamcity commented Apr 23, 2019

Uh oh!

ajwerner commented Apr 23, 2019

Uh oh!

ajwerner commented Apr 23, 2019

Uh oh!

nvb left a comment

Uh oh!

ajwerner left a comment

Uh oh!

ajwerner commented Apr 24, 2019

Uh oh!

nvb left a comment

Uh oh!

nvb left a comment

Uh oh!

ajwerner commented Apr 24, 2019

Uh oh!

craig bot commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ajwerner commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Apr 23, 2019

Uh oh!

ajwerner commented Apr 23, 2019

Uh oh!

ajwerner commented Apr 23, 2019

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner commented Apr 24, 2019

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

ajwerner commented Apr 24, 2019

Uh oh!

craig bot commented Apr 24, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ajwerner commented Apr 23, 2019 •

edited

Loading