Skip to content

Initial commit to support cancelling a long running query (WIP)#9141

Closed
kishoreg wants to merge 1 commit intomasterfrom
cancel-request
Closed

Initial commit to support cancelling a long running query (WIP)#9141
kishoreg wants to merge 1 commit intomasterfrom
cancel-request

Conversation

@kishoreg
Copy link
Member

@kishoreg kishoreg commented Aug 1, 2022

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:

  1. The PR has to be tagged with at least one of the following labels (*):
    1. feature
    2. bugfix
    3. performance
    4. ui
    5. backward-incompat
    6. release-notes (**)
  2. Remove these instructions before publishing the PR.

(*) Other labels to consider:

  • testing
  • dependencies
  • docker
  • kubernetes
  • observability
  • security
  • code-style
  • extension-point
  • refactor
  • cleanup

(**) Use release-notes label for scenarios like:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed

queryOptions.put(Broker.Request.TRACE, "true");
}

queryOptions.put(CommonConstants.Query.Request.MetadataKeys.REQUEST_ID, String.valueOf(requestId));
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@Jackie-Jiang
Copy link
Contributor

Replaced with #9171

@Jackie-Jiang Jackie-Jiang deleted the cancel-request branch August 16, 2022 17:51
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