kvserver/rangelog: extract a package, use KVs #89623
Conversation
|
This change is |
ajwerner
force-pushed
the
ajwerner/rangelog-extraction
branch
3 times, most recently
from
95d2781 to
de47b9b
Compare
The old name was too ambiguous. Release note: None
ajwerner
force-pushed
the
ajwerner/rangelog-extraction
branch
2 times, most recently
from
4955d2d to
4b6edb1
Compare
There was a problem hiding this comment.
LGTM overall, but if we could decouple this further from the SQL layer by not depending on the SQL instance ID, that would be even better. Perhaps use the KV node ID instead of SQL instance ID? After all, the rangelog queries only run on the KV nodes where there may not be a SQL instance ID at all to go with.
Reviewed 3 of 3 files at r1, 3 of 3 files at r2, 9 of 9 files at r3, 16 of 16 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)
-- commits line 27 at r4:
nit: of the interface.
pkg/server/server.go line 643 at r4 (raw file):
rangeLogWriter := rangelog.NewWriter( func() int64 { return int64(builtins.GenerateUniqueInt(idContainer.SQLInstanceID()))
Does it matter that we use the SQL instance ID for this? Does the locality property count? Could we not use purely random numbers, or perhaps even a UUID?
Code quote (from pkg/kv/kvserver/rangelog/rangelog.go):
NewWriterpkg/kv/kvserver/rangelog/rangelog_test.go line 68 at r5 (raw file):
defer s.Stopper().Stop(ctx) // Inject two table to write into. The first table will be written into by
nit: two tables
There was a problem hiding this comment.
The KV node ID is the SQL instance ID in the system tenant. I think that's actually sort of bad because of repaving problems, and if we ever did change it to use the same SQL instance ID infrastructure, I think that'd be good.
Reviewable status:
pkg/server/server.go line 643 at r4 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
Does it matter that we use the SQL instance ID for this? Does the locality property count? Could we not use purely random numbers, or perhaps even a UUID?
- we cannot change the structure of the table without significant effort, so I'm inclined not to change that
- The locality property doesn't really matter because the timestamp is in the primary key. However, I, for one, actively do not want to change the column types and I need a unique identifier.
- It seems to me like what you're quarelling with is the term
SQLInstanceIDwhich in kv nodes we always assume to be the node ID -- so these are the same thing for the moment. If it placates you, I'll happily either use the node ID and cast it or change some signatures aroundGenerateUniqueIntand cast the node ID to int32 or something like that.
There was a problem hiding this comment.
Reviewable status:
pkg/server/server.go line 643 at r4 (raw file):
Previously, ajwerner wrote…
- we cannot change the structure of the table without significant effort, so I'm inclined not to change that
- The locality property doesn't really matter because the timestamp is in the primary key. However, I, for one, actively do not want to change the column types and I need a unique identifier.
- It seems to me like what you're quarelling with is the term
SQLInstanceIDwhich in kv nodes we always assume to be the node ID -- so these are the same thing for the moment. If it placates you, I'll happily either use the node ID and cast it or change some signatures aroundGenerateUniqueIntand cast the node ID to int32 or something like that.
ok yes let's use the node ID instead of SQL instance ID. thanks
ajwerner
force-pushed
the
ajwerner/rangelog-extraction
branch
from
4b6edb1 to
ce0d8de
Compare
ajwerner
requested review from
a team,
benbardin and
stevendanna
and removed request for
a team
There was a problem hiding this comment.
I added a commit to try to make it all more explicit. Does it please you?
Reviewable status:
Previously, knz (Raphael 'kena' Poss) wrote…
nit: of the interface.
Done.
pkg/kv/kvserver/rangelog/rangelog_test.go line 68 at r5 (raw file):
Previously, knz (Raphael 'kena' Poss) wrote…
nit: two tables
Done.
There was a problem hiding this comment.
Reviewed 7 of 30 files at r6, 1 of 3 files at r7, 4 of 9 files at r8, 12 of 16 files at r9, 6 of 6 files at r10, all commit messages.
Reviewable status:
The argument used to be a base.SQLInstanceID. In some cases, we were casting to that type in order to call that function. This seems misleading. In fact, the function just needs some integer which is unique to this process in the cluster in order to achieve its goals. We add a new type ProcessUniqueID to which we can cast IDs like SQLInstanceID and NodeID without having to think too hard. Release note: None
This introduces a new interface and some logic to extract the implementation. Release note: None
Release note: None
This commit is primarily about building out testing infrastructure for the rangelog package. It does that by taking real rangelog data, encoding it as protobuf for cross-version compatibility, and then making sure it round-trips. These tools will be useful to validate an alternative implementation of the interface. Release note: None
This commit swaps out the old internal executor based implementation with one which hard-codes its understanding of the rangelog table and uses the KV API directly. Release note: None
ajwerner
force-pushed
the
ajwerner/rangelog-extraction
branch
from
ce0d8de to
285e414
Compare
|
TFTR! bors r+ |
|
Build succeeded: |
This sequence of commits extracts the rangelog logic to a subpackage, refactors the ID generation logic for more
control, adds some testing data and infrastructure, and, finally, swaps out the underlying implementation to use
raw KVs.
Fixes #89164
Fixes #35293
Release note: None