kv: Use strict types for common fields #100181

andrewbaptist · 2023-03-30T18:04:05Z

This PR introduces 3 new typed fields in mvcc.go:
RaftTerm, RaftIndex and LeaseSequence. These fields were previously just unit64 throughout the code and this made the code harder to read and risked incorrect conversions.

Epic: none

Release note: None

cockroach-teamcity · 2023-03-30T18:04:13Z

This change is

andrewbaptist · 2023-03-31T20:05:59Z

This was a 95% mechanical change and seemed pretty useful.

There are three things that I don't love about this PR but couldn't easily change:

The etc.d raft library doesn't strongly type the Term or Index, so it's necessary to cast for all the places that interact with that. There aren't too many of them, and they are usually very clear what the type is, so I don't think this is a big problem.
I needed to put the type definitions in the enginepb library so they could be used everywhere. I originally planned on putting them in roachpb but that doesn't work because of RangeAppliedState which is in mvcc3.proto
The LeaseSequence was inconsistently a uint64 or a int64. I made the type int64 to make sure there weren't any problems with negative numbers, but it probably doesn't matter which one it is.

Let me know if you have any thoughts on this.

Finally I put a few reviewers on here, but I don't expect everyone to look at every line. I'm not sure the best way to tag it.

andrewbaptist · 2023-03-31T20:08:01Z

Also if we decide to make this change should be backported? It feels pretty low risk and could avoid some merge conflicts on backports. However, it probably isn't that important to do as the conflicts would be trivial to resolve. There are no serialization format changes to any protobufs.

erikgrinaker · 2023-04-03T10:02:44Z

I needed to put the type definitions in the enginepb library so they could be used everywhere. I originally planned on putting them in roachpb but that doesn't work because of RangeAppliedState which is in mvcc3.proto

Let's move RangeAppliedState and LeaseSequence into kvserverpb/state.{proto,go}, and the Raft types into kvserverpb/raft.{proto,go}. None of this has anything to do with MVCC. Hopefully that'll be straightforward.

The LeaseSequence was inconsistently a uint64 or a int64. I made the type int64 to make sure there weren't any problems with negative numbers, but it probably doesn't matter which one it is.

Since we're doing all of this work anyway, let's just flip it to uint64. I think a negative lease sequence would have stuff blowing up anyway, and they should be wire-compatible (but I've been wrong about Protobuf changes before).

Also if we decide to make this change should be backported?

I'd prefer not to. There's only downside to users, if we get something wrong.

andrewbaptist · 2023-04-03T23:56:14Z

Let's move RangeAppliedState and LeaseSequence into kvserverpb/state.{proto,go}, and the Raft types into kvserverpb/raft.{proto,go}. None of this has anything to do with MVCC. Hopefully that'll be straightforward.

I was able to move them into kvserverpb at least, but it would have taken a fair bit more rearranging to move then into the more specific packages. This would probably be a good thing to do as a follow-on PR later.

Since we're doing all of this work anyway, let's just flip it to uint64. I think a negative lease sequence would have stuff blowing up anyway, and they should be wire-compatible (but I've been wrong about Protobuf changes before).

I couldn't do this without breaking TestBelowRaftProtosDontChange. I tried a couple of variations of this, but it wasn't possible to make it a uint64 but serialize as a int64 either since it hit a negative check.

I'd prefer not to. There's only downside to users, if we get something wrong.

Sounds good - I won't plan it for backport. The only risk I saw was making some PRs harder to backport, but I think that is not overly important.

erikgrinaker

I was able to move them into kvserverpb at least, but it would have taken a fair bit more rearranging to move then into the more specific packages. This would probably be a good thing to do as a follow-on PR later.

That's fine, we can clean it up later. I take it we were forced to move the index/term types into roachpb for similar dependency reasons?

I couldn't do this without breaking TestBelowRaftProtosDontChange. I tried a couple of variations of this, but it wasn't possible to make it a uint64 but serialize as a int64 either since it hit a negative check.

I think that's just because the test uses Protobuf-generated Populate* functions that randomly populate messages using a fixed seed. When we change a field from int64 to uint64 it can both change the randomly chosen values, and possibly also inject additional calls to the RNG such as if r.Intn(2) == 0 { this.Sequence *= -1 }. It can therefore change both the randomly generated values and the sequence, but that doesn't mean that the messages aren't wire-compatible.

However, when I tried changing it to uint64 and running TestBelowRaftProtosDontChange it passed. 🤷‍♀️

Reviewed 40 of 119 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist, @nvanbenschoten, @pavelkalinnikov, @RaduBerinde, and @tbg)

-- commits line 4 at r2:
Needs updating.

pkg/kv/kvpb/api.proto line 2102 at r2 (raw file):

  // applied index of the CPut that left an intent on the local copy of the
  // right-hand range descriptor.
  int64 lease_applied_index = 4  [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.LeaseSequence"];

The lease applied index is not a lease sequence number, so this type isn't appropriate. Didn't we already have a separate type for it?

The lease sequence number distinguishes different leases from each other. It is incremented when we acquire a new lease.

The lease applied index is used to order individual Raft proposals, for replay protection. It is incremented for every Raft proposal:

cockroach/pkg/kv/kvserver/replica_proposal_buf.go

Line 767 in 6e3a111

b.assignedLAI++

pkg/kv/kvserver/kvserverpb/state.proto line 40 at r2 (raw file):

  uint64 raft_applied_index = 1 [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.RaftIndex"];
  // The highest (and last) lease index applied to the state machine.
  int64 lease_applied_index = 2  [(gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.LeaseSequence"];

As mentioned elsewhere, this and other instances should be a LeaseAppliedIndex not a LeaseSequence.

andrewbaptist · 2023-04-04T20:28:18Z

Thanks! I didn't realize the difference between LeaseSequence and LeaseAppliedIndex until now. This makes more sense.

I updated to use uint64 everywhere and it is passing the tests now. I must have been doing something strange before, but it seems like you were right that we can do this.

Finally, I wanted to move all these fields into kv so I looked at what was necessary to do that. It turned out that only the internal_raft.proto was holding this up, and that didn't make sense in roachpb anyway. I moved that over and updated the PR to have the fields in better locations.

erikgrinaker · 2023-04-05T12:59:24Z

Thanks! I won't have time for another pass before OOO and breather week, but I suppose we'd want to let this sit until the 23.1 backport bonanza calms down anyway.

andrewbaptist · 2023-04-05T13:37:07Z

Yep, we can let this sit a little. There isn't a rush to get this in. It shouldn't be too hard to keep this up-to-date with master. At least it is now all green.

andrewbaptist · 2023-04-21T22:49:29Z

@erikgrinaker if you get a chance next week can you review this again? Would be nice to merge to master now that backports to 23.1 have slowed down.

tbg

LGTM

Don't bother addressing the import nits in this PR if it's green, can be done as a follow-up PR.

tbg · 2023-04-26T13:19:36Z

pkg/kv/kvpb/data.go

+type RaftIndex uint64
+
+// SafeValue implements the redact.SafeValue interface.
+func (s RaftTerm) SafeValue()  {}


nit: it's unusual to implement this "not next to" the receiver.

tbg · 2023-04-26T13:20:17Z

pkg/kv/kvserver/closedts/ctpb/service.go

 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 )

+// Used within the RangeUpdate method.
+var _ kvpb.LeaseAppliedIndex


Why not remove this? This is a no-op.

If you need kvpb imported, use a _ "github.com/cockroachdb/cockroach/pkg/kv/kvpb" import instead.

tbg · 2023-04-26T13:21:17Z

pkg/kv/kvserver/kvserverpb/state.proto

+// mvcc stats. These are all persisted on each transition of the Raft
+// state machine (i.e. on each Raft application), so they are stored
+// in the same RocksDB key for efficiency.
+message RangeAppliedState {


Moved from elsewhere with just casttype adjustments, right?

tbg · 2023-04-26T13:21:34Z

pkg/kv/kvserver/loqrecovery/loqrecoverypb/recovery.go

@@ -25,6 +26,9 @@ import (
 // representation outside of tests.
 type RecoveryKey roachpb.RKey

+// Needed for recovery.proto.
+var _ kvpb.RaftIndex


andrewbaptist

No problem - all easy to address. TFTY

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @nvanbenschoten, @pavelkalinnikov, @RaduBerinde, and @tbg)

-- commits line 4 at r2:

Previously, erikgrinaker (Erik Grinaker) wrote…

Needs updating.

Updated - thanks.

pkg/kv/kvpb/api.proto line 2102 at r2 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

The lease applied index is not a lease sequence number, so this type isn't appropriate. Didn't we already have a separate type for it?

The lease sequence number distinguishes different leases from each other. It is incremented when we acquire a new lease.

The lease applied index is used to order individual Raft proposals, for replay protection. It is incremented for every Raft proposal:

cockroach/pkg/kv/kvserver/replica_proposal_buf.go

Line 767 in 6e3a111

b.assignedLAI++

Fixed. I changed to separate these back out.

pkg/kv/kvpb/data.go line 174 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: it's unusual to implement this "not next to" the receiver.

Fixed, moved next to the definitions.

pkg/kv/kvserver/closedts/ctpb/service.go line 25 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

If you need kvpb imported, use a _ "github.com/cockroachdb/cockroach/pkg/kv/kvpb" import instead.

Done. Thanks for the tip!

pkg/kv/kvserver/kvserverpb/state.proto line 40 at r2 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

As mentioned elsewhere, this and other instances should be a LeaseAppliedIndex not a LeaseSequence.

Changed

pkg/kv/kvserver/kvserverpb/state.proto line 218 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Moved from elsewhere with just casttype adjustments, right?

Correct. This was in roachpb before, but more appropriate here.

pkg/kv/kvserver/loqrecovery/loqrecoverypb/recovery.go line 30 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Ditto

Fixed

This PR introduces 3 new typed fields to the kvpb package: RaftTerm, RaftIndex and LeaseAppliedIndex. These fields were previously just unit64 throughout the code and this made the code harder to read and risked incorrect conversions. It also moves internal_raft.proto into the kvserver package as it is no longer accessed outside of kv. Epic: none Release note: None

erikgrinaker

Thanks for the cleanup! Reviewed all of the Protobuf changes, LGTM.

For next time, it'd be nice to split this out into a few commits (e.g. file reorganization and Protobuf message changes), to more easily identify the changes that need particular scrutiny. But don't spend time on that now.

andrewbaptist · 2023-04-28T12:48:32Z

bors r=erikgrinaker

TFTR - I'll split it out any future cleanups.

craig · 2023-04-28T13:32:18Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-04-28T14:34:13Z

Build succeeded:

Bazel Essential CI (Cockroach)

andrewbaptist force-pushed the 2023-03-29-strict-types branch 9 times, most recently from 2ae342b to d607489 Compare March 31, 2023 20:05

andrewbaptist requested review from erikgrinaker, pav-kv, tbg and nvanbenschoten March 31, 2023 20:05

andrewbaptist marked this pull request as ready for review March 31, 2023 20:06

andrewbaptist requested review from a team as code owners March 31, 2023 20:06

andrewbaptist requested a review from a team March 31, 2023 20:06

andrewbaptist requested a review from a team as a code owner March 31, 2023 20:06

andrewbaptist requested a review from RaduBerinde March 31, 2023 20:06

andrewbaptist force-pushed the 2023-03-29-strict-types branch 3 times, most recently from 9c0f8ec to 6e3a111 Compare April 3, 2023 20:08

erikgrinaker reviewed Apr 4, 2023

View reviewed changes

andrewbaptist force-pushed the 2023-03-29-strict-types branch 2 times, most recently from a89d330 to caa73c2 Compare April 4, 2023 17:18

andrewbaptist force-pushed the 2023-03-29-strict-types branch 3 times, most recently from f88b416 to 0d49b3a Compare April 4, 2023 20:16

andrewbaptist force-pushed the 2023-03-29-strict-types branch 5 times, most recently from ec12863 to 16e5036 Compare April 5, 2023 12:57

andrewbaptist mentioned this pull request Apr 18, 2023

Add metrics and type-saftey assertions about the raft indexes in delegate snapshot requests #98243

Closed

andrewbaptist force-pushed the 2023-03-29-strict-types branch 2 times, most recently from 9d1269a to d5a9005 Compare April 21, 2023 22:31

andrewbaptist requested a review from erikgrinaker April 21, 2023 22:49

tbg approved these changes Apr 26, 2023

View reviewed changes

andrewbaptist force-pushed the 2023-03-29-strict-types branch from d5a9005 to a8f676b Compare April 27, 2023 13:43

andrewbaptist commented Apr 27, 2023

View reviewed changes

andrewbaptist force-pushed the 2023-03-29-strict-types branch from a8f676b to a9989a8 Compare April 27, 2023 15:41

erikgrinaker approved these changes Apr 28, 2023

View reviewed changes

craig bot merged commit 8203394 into cockroachdb:master Apr 28, 2023

andrewbaptist deleted the 2023-03-29-strict-types branch April 29, 2023 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kv: Use strict types for common fields #100181

kv: Use strict types for common fields #100181

andrewbaptist commented Mar 30, 2023

cockroach-teamcity commented Mar 30, 2023

andrewbaptist commented Mar 31, 2023

andrewbaptist commented Mar 31, 2023

erikgrinaker commented Apr 3, 2023

andrewbaptist commented Apr 3, 2023

erikgrinaker left a comment

andrewbaptist commented Apr 4, 2023

erikgrinaker commented Apr 5, 2023

andrewbaptist commented Apr 5, 2023

andrewbaptist commented Apr 21, 2023

tbg left a comment

tbg Apr 26, 2023

tbg Apr 26, 2023

tbg Apr 26, 2023

tbg Apr 26, 2023

tbg Apr 26, 2023

andrewbaptist left a comment

erikgrinaker left a comment •

edited

Loading

andrewbaptist commented Apr 28, 2023

craig bot commented Apr 28, 2023

craig bot commented Apr 28, 2023

kv: Use strict types for common fields #100181

kv: Use strict types for common fields #100181

Conversation

andrewbaptist commented Mar 30, 2023

cockroach-teamcity commented Mar 30, 2023

andrewbaptist commented Mar 31, 2023

andrewbaptist commented Mar 31, 2023

erikgrinaker commented Apr 3, 2023

andrewbaptist commented Apr 3, 2023

erikgrinaker left a comment

Choose a reason for hiding this comment

andrewbaptist commented Apr 4, 2023

erikgrinaker commented Apr 5, 2023

andrewbaptist commented Apr 5, 2023

andrewbaptist commented Apr 21, 2023

tbg left a comment

Choose a reason for hiding this comment

tbg Apr 26, 2023

Choose a reason for hiding this comment

tbg Apr 26, 2023

Choose a reason for hiding this comment

tbg Apr 26, 2023

Choose a reason for hiding this comment

tbg Apr 26, 2023

Choose a reason for hiding this comment

tbg Apr 26, 2023

Choose a reason for hiding this comment

andrewbaptist left a comment

Choose a reason for hiding this comment

erikgrinaker left a comment • edited Loading

Choose a reason for hiding this comment

andrewbaptist commented Apr 28, 2023

craig bot commented Apr 28, 2023

craig bot commented Apr 28, 2023

erikgrinaker left a comment •

edited

Loading