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

kvserver: return destination from AdminScatter #51172

Closed
wants to merge 1 commit into from

Conversation

pbardea
Copy link
Contributor

@pbardea pbardea commented Jul 8, 2020

This commit updates the AdminScatter request to return the node to which
the range that was scattere was sent. This is information is used during
restore's distsql flow.

Release note: None

This commit updates the AdminScatter request to return the node to which
the range that was scattere was sent. This is information is used during
restore's distsql flow.

Release note: None
@pbardea pbardea requested a review from dt July 8, 2020 20:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pbardea
Copy link
Contributor Author

pbardea commented Jul 8, 2020

Actually thinking more about this, it may be cleaner if we just issue a GetLeaseInfo request from the call side in restore. I don't think that this would request would dramatically slow down restore and would keep the API tidier. Curious about your thoughts here, but otherwise I'm tempted to close this out.

@dt
Copy link
Member

dt commented Jul 8, 2020

I think it is reasonable for "please scatter this thing" to reply with "okay, i scattered it to X" instead of just "okay", but my opinion isn't really the one that matters here. Adding some kv folks.

@dt dt requested review from andreimatei and ajwerner July 8, 2020 20:29
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Are you familiar with the ReturnRangeInfo request header field? It seems like it should get populated with the lease as of the end of the command which for AdminScatter will be the post-scatter lease IIUC.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @andreimatei, and @dt)

@pbardea
Copy link
Contributor Author

pbardea commented Jul 8, 2020

Ah, yep - I took a gander through the header but missed that. That's exactly what I was reaching for - thanks for the pointer!

@pbardea pbardea closed this Jul 8, 2020
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.

I'm currently getting rid of ReturnRangeInfo, replacing it with something else: instead of explicitly asking for range info, the client is always gonna send info about what it believes the state of the range to be and the server is gonna reply with updates if it has any.
I don't think you really want to use this mechanism here... Returning the state of the lease after a request has run is not exactly what this was designed for. Plus, these RangeInfo responses will be terminated at the DistSender, they shouldn't make sense to a higher layer.

I think it'd be much better if you add an explicit response for AdminScatter. And btw remind me pls how this scatter works - is it a range request (and so we rely on the DistSender to split it) or a point request? I think it's a range, but then I'm confused about why you had a single target_node in the response and you didn't have to handle the merging of results in the DistSender.

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

@pbardea
Copy link
Contributor Author

pbardea commented Jul 20, 2020

Sorry I missed your comment. AdminScatter is indeed a range request. I've updated the interface so that the destination node ID is returned per-range, so that DistSender can continue to combine the ranges as is.

@pbardea
Copy link
Contributor Author

pbardea commented Jul 20, 2020

Github seems to have gotten confused when updating a closed PR. Created #51607.

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.

5 participants