Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kv: complete client range info migrations #59395

Merged

Conversation

nvanbenschoten
Copy link
Member

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/clientRangeInfo branch from e4d29e5 to e5f2e43 Compare January 25, 2021 19:52
Copy link
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.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/clientRangeInfo branch from e5f2e43 to e2c1477 Compare January 26, 2021 22:08
Copy link
Member Author

@nvanbenschoten nvanbenschoten 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
Contributor

craig bot commented Jan 26, 2021

Build succeeded:

@craig craig bot merged commit 854246a into cockroachdb:master Jan 26, 2021
@nvanbenschoten nvanbenschoten 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