kvserver: make RangeStatsRequest return the desc explicitly#51378
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Jul 15, 2020
Merged
Conversation
Member
|
This change is |
nvb
approved these changes
Jul 14, 2020
Contributor
nvb
left a comment
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andreimatei and @tbg)
pkg/kv/kvserver/batcheval/cmd_range_stats.go, line 36 at r1 (raw file):
func init() { RegisterReadOnlyCommand(roachpb.RangeStats, DefaultDeclareKeys, RangeStats)
s/DefaultDeclareKeys/declareKeysRangeStats/
Hopefully this was caught by testrace.
bbb2151 to
8a36ec7
Compare
andreimatei
commented
Jul 14, 2020
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvserver/batcheval/cmd_range_stats.go, line 36 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
s/DefaultDeclareKeys/declareKeysRangeStats/Hopefully this was caught by testrace.
it was
Before this patch, the main user of RangeStatsRequest would ask to get the range's descriptor through the batches' ReturnRangeInfo field. I'm trying to get rid of the ReturnRangeInfo field because it's pretty hacky: it's a batch field, not an individual request field, which means that the results are not associated with a particular request. Also, the server does not acquire latches on the range's descriptor/lease keys, which means that the request is not serialized with others changing this data, which in turn may or may not be a problem for any particular caller. This patch makes the RangeStatsRequest return descriptor/lease info explicitly, and acquire said latches. The API is now cleaner for the users of this request. Release note: None
8a36ec7 to
1c993e0
Compare
andreimatei
commented
Jul 14, 2020
Contributor
Author
andreimatei
left a comment
There was a problem hiding this comment.
bors r+
Reviewable status:
Contributor
Build succeeded |
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
Jan 25, 2021
This commit completes a series of migrations relating to RangeInfo state and the client/server protocol for keeping the range descriptor cache up to date. The migrations were started in cockroachdb#51168, cockroachdb#51378, cockroachdb#51437. Concretely, this commit removes the following cluster versions: - `ClientRangeInfosOnBatchResponse` - `RangeStatsRespHasDesc` It addresses 6 TODOs. And it makes the following changes to the KV APi: - makes `Header.ClientRangeInfo` non-nullable. - deletes `Header.ReturnRangeInfo`. - deletes `ResponseHeader.DeprecatedRangeInfos`. - stops propagating `BatchResponse.RangeInfos` past `DistSender`. - makes RangeStatsResponse.RangeInfo non-nullable.
nvb
added a commit
to nvb/cockroach
that referenced
this pull request
Jan 26, 2021
This commit completes a series of migrations relating to RangeInfo state and the client/server protocol for keeping the range descriptor cache up to date. The migrations were started in cockroachdb#51168, cockroachdb#51378, cockroachdb#51437. Concretely, this commit removes the following cluster versions: - `ClientRangeInfosOnBatchResponse` - `RangeStatsRespHasDesc` It addresses 6 TODOs. And it makes the following changes to the KV APi: - makes `Header.ClientRangeInfo` non-nullable. - deletes `Header.ReturnRangeInfo`. - deletes `ResponseHeader.DeprecatedRangeInfos`. - stops propagating `BatchResponse.RangeInfos` past `DistSender`. - makes RangeStatsResponse.RangeInfo non-nullable.
craig bot
pushed a commit
that referenced
this pull request
Jan 26, 2021
59395: kv: complete client range info migrations r=nvanbenschoten a=nvanbenschoten This commit completes a series of migrations relating to RangeInfo state and the client/server protocol for keeping the range descriptor cache up to date. The migrations were started in #51168, #51378, #51437. Concretely, this commit removes the following cluster versions: - `ClientRangeInfosOnBatchResponse` - `RangeStatsRespHasDesc` It addresses 6 TODOs. And it makes the following changes to the KV APi: - makes `Header.ClientRangeInfo` non-nullable. - deletes `Header.ReturnRangeInfo`. - deletes `ResponseHeader.DeprecatedRangeInfos`. - stops propagating `BatchResponse.RangeInfos` past `DistSender`. - makes RangeStatsResponse.RangeInfo non-nullable. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this patch, the main user of RangeStatsRequest would ask to get
the range's descriptor through the batches' ReturnRangeInfo field. I'm
trying to get rid of the ReturnRangeInfo field because it's pretty
hacky: it's a batch field, not an individual request field, which means
that the results are not associated with a particular request. Also, the
server does not acquire latches on the range's descriptor/lease keys,
which means that the request is not serialized with others changing
this data, which in turn may or may not be a problem for any particular
caller.
This patch makes the RangeStatsRequest return descriptor/lease info
explicitly, and acquire said latches. The API is now cleaner for the
users of this request.
Release note: None