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

[no-release-notes] go: sqle,remotesrv: Implement sql.Session lifecycle callbacks for sql.Contexts used in remotesrv RPCs. #8797

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

reltuk
Copy link
Contributor

@reltuk reltuk commented Jan 28, 2025

This PR changes each RPC invocation against the gRPC and HTTP servers
implementing remotesapi and cluster replication to create a sql.Context
which lives the duration of the call. The Session for that call gets
SessionCommand{Begin,End} and SessionEnd lifecycle callbacks so that it
can participate in GC safepoint rendezvous appropriately.

Previously the remotesrv.DBCache and the user/password remotesapi
authentication implementation would simply create new sql.Contexts
whenever they needed them. There could be multiple sql.Contexts for the
single server call.

…System, remove the confusing indirection through ctxFactory.
….Contexts used in remotesrv RPCs.

This PR changes each RPC invocation against the gRPC and HTTP servers
implementing remotesapi and cluster replication to create a sql.Context
which lives the duration of the call. The Session for that call gets
SessionCommand{Begin,End} and SessionEnd lifecycle callbacks so that it
can participate in GC safepoint rendezvous appropriately.

Previously the remotesrv.DBCache and the user/password remotesapi
authentication implementation would simply create new sql.Contexts
whenever they needed them. There could be multiple sql.Contexts for the
single server call.
@reltuk reltuk requested a review from max-hoffman January 28, 2025 22:32
@coffeegoddd
Copy link
Contributor

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000
version result total
a6b1a26 ok 5937457
version total_tests
a6b1a26 5937457
correctness_percentage
100.0

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

The handlers look good. I guess the main gap for me is whether cluster/remotesrv use of GetInterceptorSqlContext is always safe. Ex: are there any other way those servers request a context that for some reason doesn't pass through the interceptor handlers, and would error in want of the context key.

go/libraries/doltcore/sqle/remotesrv.go Outdated Show resolved Hide resolved
Co-authored-by: Maximilian Hoffman <[email protected]>
@reltuk
Copy link
Contributor Author

reltuk commented Jan 30, 2025

The handlers look good. I guess the main gap for me is whether cluster/remotesrv use of GetInterceptorSqlContext is always safe. Ex: are there any other way those servers request a context that for some reason doesn't pass through the interceptor handlers, and would error in want of the context key.

This is a good question. This work covered inbound replication, and I think it got all the moving pieces for that part. But your question makes me think of outbound replication, in particular when/if the pushes are asynchronous, and in the context of both cluster replication and for push-on-write. I will take a pass at seeing what the state for session lifecycle stuff is there.

@coffeegoddd
Copy link
Contributor

@reltuk DOLT

comparing_percentages
100.000000 to 100.000000
version result total
ddaa922 ok 5937457
version total_tests
ddaa922 5937457
correctness_percentage
100.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants