server: Tenant pods fan out to cancel SQL queries #71503
Conversation
|
This change is |
There was a problem hiding this comment.
as long as the tests are fixed
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @rafiss)
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, all commit messages.
Reviewable status:
pkg/ccl/serverccl/tenant_status_test.go, line 600 at r2 (raw file):
cancelQueryReq := serverpb.CancelQueryRequest{QueryID: query.ID} cancelQueryResp := serverpb.CancelQueryResponse{} err = httpPod1.PostJSON("/_status/cancel_query/"+session.NodeID.String(), &cancelQueryReq, &cancelQueryResp)
hmm, is the NodeID args here necessary? We would just fanout the RPC anyway right ?
pkg/ccl/serverccl/tenant_status_test.go, line 602 at r2 (raw file):
err = httpPod1.PostJSON("/_status/cancel_query/"+session.NodeID.String(), &cancelQueryReq, &cancelQueryResp) require.NoError(t, err) require.Equal(t, true, cancelQueryResp.Canceled, cancelQueryResp.Error)
super small nit: perhaps we can format the error message a bit, e.g. "expected query to be cancelled, but encountered unexpected error %s"
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/serverccl/tenant_status_test.go, line 600 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
hmm, is the
NodeIDargs here necessary? We would just fanout the RPC anyway right ?
Good call. It had been necessary before I switched to fanout (I was jamming a SQLInstanceID into that field), but, yes, now that we're fanning out, I've replaced it with a 0 here.
pkg/ccl/serverccl/tenant_status_test.go, line 602 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
super small nit: perhaps we can format the error message a bit, e.g. "expected query to be cancelled, but encountered unexpected error %s"
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/serverccl/tenant_status_test.go, line 600 at r2 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Good call. It had been necessary before I switched to fanout (I was jamming a SQLInstanceID into that field), but, yes, now that we're fanning out, I've replaced it with a
0here.
Ah nice. Hmm just wondering is it possible we can just leave it blank? I think 0 is an invalid SQLInstanceID right ?
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/serverccl/tenant_status_test.go, line 600 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Ah nice. Hmm just wondering is it possible we can just leave it blank? I think
0is an invalid SQLInstanceID right ?
Good question! I just looked, and the grpc gateway won't let us leave it blank. I did wonder about putting some "meaningful" value in here, but I didn't have any ideas I liked, since the handler actually ignores this field altogether. Any thoughts?
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/serverccl/tenant_status_test.go, line 600 at r2 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Good question! I just looked, and the grpc gateway won't let us leave it blank. I did wonder about putting some "meaningful" value in here, but I didn't have any ideas I liked, since the handler actually ignores this field altogether. Any thoughts?
Oooof 😢 , this is unfortunate.
Actually, i'm not entirely sure if node_Id needs to be in the path param 🤔 . /_status/resetsqlstats has NodeID in the request payload but it doesn't have it specified in the path param. This is because it's a POST endpoint and we directly pass in the request protobuf from the caller. So perhaps that would be a way for us to drop the node_id field from the URL. Also I think for /_status/statements endpoint we have been using query params instead of the path params so that node_id will become optional.
I guess that means we also need to update the frontend code to use those as well which can be a bit annoying 😅 . So I will defer that decision to you
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/serverccl/tenant_status_test.go, line 600 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Oooof 😢 , this is unfortunate.
Actually, i'm not entirely sure if
node_Idneeds to be in the path param 🤔 ./_status/resetsqlstatshasNodeIDin the request payload but it doesn't have it specified in the path param. This is because it's a POST endpoint and we directly pass in the request protobuf from the caller. So perhaps that would be a way for us to drop thenode_idfield from the URL. Also I think for/_status/statementsendpoint we have been using query params instead of the path params so thatnode_idwill become optional.I guess that means we also need to update the frontend code to use those as well which can be a bit annoying 😅 . So I will defer that decision to you
Agreed! Yes, that would work. I don't see a design reason why the node_id was put in the URL. But since that's where it also lives for the dedicated (non-tenant) status server, I suggest we not change it in this pull request. SG?
There was a problem hiding this comment.
Reviewable status:
Partially addresses #70832. Previously, the tenant `/_status/cancel_query` endpoint was not available over HTTP and could not cancel a query not running locally. This change opens it up, so that we can call it from the cloud console, and makes sure that that we can cancel queries when a tenant has more than one SQL pod running. Release note (api change): The tenant `/_status/cancel_query` endpoint was made available over HTTP and augmented to fan out requests in a multi-pod world.
|
bors r+ |
|
Build succeeded: |
Partially addresses #70832.
Previously, the tenant
/_status/cancel_queryendpoint was notavailable over HTTP and could not cancel a query not running locally.
This change opens it up, so that we can call it from the cloud console,
and makes sure that that we can cancel queries when a tenant has more
than one SQL pod running.
Release note (api change): The tenant
/_status/cancel_queryendpointwas made available over HTTP and augmented to fan out requests in a
multi-pod world.