storage: invalidate cached lastTerm on snapshots

[nvanbenschoten]

Addresses the current issue in #17524.

I'll open an issue to properly test this, but that might take some time.
Fow now this seems like a clear fix that we should get in sooner rather
than later.

[cockroach-teamcity]

This change is 

[tbg]

Looks good, but it strikes me as brittle that we have two values that are tied together but are kept separate.

Maybe a struct lastTermAndIndex could make it more obvious that changing one without the other is illegal. Just a thought though.

[tbg]

couldn't you just return struct lastIndexAndTerm from applySnapshot and save the other call to lastIndexAndTermInSnapshot? Nit, but I think it might be clearer.

[petermattis]

modulo not changing where we retrieve lastIndex from following a snapshot.

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/storage/replica.go, line 3254 at r1 (raw file):

		if lastIndex, err = r.raftMu.stateLoader.loadLastIndex(ctx, r.store.Engine()); err != nil {
			return stats, err
		}

For a 1.1 cherry-pick, I'd prefer doing lastTerm = invalidLastTerm and stick with loading lastIndex from disk.

---

Comments from Reviewable

[nvanbenschoten]

Heh, two opposite recommendations. Should I split it up into two commits, one with just the one line change and one with the rest? Then we can just cherry-pick in the first one.

[tbg]

I don't feel strongly, we could also just do the beauty refactor later in the cycle if we track it. Getting a nice contained cherry-pick is the more important item.

On Thu, Sep 7, 2017 at 12:40 PM Nathan VanBenschoten < ***@***.***> wrote: Heh, two opposite recommendations. Should I split it up into two commits, one with just the one line change and one with the rest? Then we can just cherry-pick in the first one. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#18327 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135CcDLYCvbyWfkFySvei4iL12sZn7ks5sgBxwgaJpZM4PQD4f> .

--

…

-- Tobias

[petermattis]

I don't feel strongly, we could also just do the beauty refactor later in
the cycle if we track it. Getting a nice contained cherry-pick is the more
important item.

Agreed on the nicely contained cherry-pick being the priority. That's what @nvanbenschoten and i discussed in person. I think it should be followed up quickly with both the beauty refactor and a test.

[nvanbenschoten]

Done. PTAL.

Opened #18336 to track cleanup and testing.

[petermattis]

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

Comments from Reviewable