Skip to content

kv: complete client range info migrations#59395

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/clientRangeInfo
Jan 26, 2021
Merged

kv: complete client range info migrations#59395
craig[bot] merged 1 commit intocockroachdb:masterfrom
nvb:nvanbenschoten/clientRangeInfo

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented 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 #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.

@nvb nvb requested a review from andreimatei January 25, 2021 19:09
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@nvb nvb force-pushed the nvanbenschoten/clientRangeInfo branch from e4d29e5 to e5f2e43 Compare January 25, 2021 19:52
Copy link
Copy Markdown
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so :lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)


pkg/kv/kvclient/kvcoord/dist_sender.go, line 1967 at r1 (raw file):

					log.VEventf(ctx, 2, "received updated range info: %s", br.RangeInfos)
					routing.EvictAndReplace(ctx, br.RangeInfos...)
					br.RangeInfos = nil

say again that this field doesn't make it past the DistSender


pkg/roachpb/api.proto, line 2029 at r1 (raw file):

    // is left empty. Not set when Error is set.
    //
    // The server may also include additional RangeInfo objects if it suspects

say that this field is cleared by the DistSender, since it referes to multi-range considerations not exposed by the kv api

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 nvb force-pushed the nvanbenschoten/clientRangeInfo branch from e5f2e43 to e2c1477 Compare January 26, 2021 22:08
Copy link
Copy Markdown
Contributor Author

@nvb nvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei)


pkg/kv/kvclient/kvcoord/dist_sender.go, line 1967 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

say again that this field doesn't make it past the DistSender

Done.


pkg/roachpb/api.proto, line 2029 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

say that this field is cleared by the DistSender, since it referes to multi-range considerations not exposed by the kv api

Done.

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 26, 2021

Build succeeded:

@craig craig bot merged commit 854246a into cockroachdb:master Jan 26, 2021
@nvb nvb deleted the nvanbenschoten/clientRangeInfo branch January 28, 2021 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants