Initial commit to support cancelling a long running query (WIP)#9141
Initial commit to support cancelling a long running query (WIP)#9141
Conversation
d66cc7e to
1f41205
Compare
| queryOptions.put(Broker.Request.TRACE, "true"); | ||
| } | ||
|
|
||
| queryOptions.put(CommonConstants.Query.Request.MetadataKeys.REQUEST_ID, String.valueOf(requestId)); |
There was a problem hiding this comment.
I assume this is just to pass requestId to servers? The requestId is already passed over, and can be read via queryRequest.getRequestId() at server side
e.g.
ServerQueryExecutorV1Impl.processQuery() {
...
try {
Tracing.getTracer().register(queryRequest.getRequestId());
...
}
| } | ||
|
|
||
| public static void cancel(long requestId) { | ||
| Pair<QueryContext, Future[]> queryContextFuturesPair = requestId2FuturesMap.get(requestId); |
There was a problem hiding this comment.
use remove() instead? to avoid keeping futures around.
alternatively, can also clean things up in unregister() method.
|
|
||
| try { | ||
| long requestId = Long.parseLong(queryContext.getQueryOptions().get("requestId")); | ||
| TraceContext.register(requestId, queryContext, _futures); |
There was a problem hiding this comment.
Would suggest not to reuse TraceContext and keep it just for tracing/monitoring purpose. Perhaps create sth similar to control query at runtime, e.g. QueryRuntimeContext to track the futures.
But in general, I'd prefer add cancel() on Operator and Plan interface to make it explicit that query is cancellable. The reason to add cancel() on Plan interface is because queryPlan.execute() is the entry point of query execution, so we can have queryPlan.cancel() to cancel a query.
|
Replaced with #9171 |
Some long-running queries will continue to hog resources.. this PR allows users to cancel a query using request id. As of now users need to know the request id. Eventually, we will have the ability to list the running queries on the controller and users can cancel them.
Instructions:
featurebugfixperformanceuibackward-incompatrelease-notes(**)(*) Other labels to consider:
testingdependenciesdockerkubernetesobservabilitysecuritycode-styleextension-pointrefactorcleanup(**) Use
release-noteslabel for scenarios like: