Remove query status report from BE when query is cancelled normally #1489
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When query result reach limit, the Coordinator in FE will send a cancel request to BE to cancel the query. And when being cancelled, BE will report query status to FE for debug purpose. But actually it is not necessary and will generate too many logs. So I add a CancelReason to distinguish the difference between 'normally' cancellation and 'internal error' cancellation. if 'normally' cancelled, no status will be reported from BE. When query reach limit, or user cancel it actively, it is being cancelled 'normally'. Otherwise, the query is cancelled due to internal error, which will need a report from BE.
imay
requested changes
Jul 17, 2019
gensrc/proto/internal_service.proto
Outdated
| @@ -116,6 +116,12 @@ message PExecPlanFragmentResult { | |||
|
|
|||
| message PCancelPlanFragmentRequest { | |||
| required PUniqueId finst_id = 1; | |||
| enum CancelReason { | |||
| LIMIT_REACH = 0; | |||
gensrc/proto/internal_service.proto
Outdated
| @@ -116,6 +116,12 @@ message PExecPlanFragmentResult { | |||
|
|
|||
| message PCancelPlanFragmentRequest { | |||
| required PUniqueId finst_id = 1; | |||
| enum CancelReason { | |||
There was a problem hiding this comment.
I think it's better to move CancelReason out of PCancelPlanFragmentRequest. Because fragment can be cancelled in other format. For example when Fragment is timed out, cancel thread will cancel this instance.
|
|
||
| // The reason of cancellation of this query. | ||
| // This is used for telling BE whether it need to report query status when being cancelled. | ||
| public enum CancelReason { |
There was a problem hiding this comment.
Because you have defined in protobuf, we should use it to avoid define one thing in different places.
be/src/runtime/fragment_mgr.cpp
Outdated
| std::lock_guard<std::mutex> l(_status_lock); | ||
| RETURN_IF_ERROR(_exec_status); | ||
| if (reason != PCancelPlanFragmentRequest::INTERNAL_ERROR) { |
There was a problem hiding this comment.
I think it's better to use equal other than not equal here. Because when we add more types of error later, we should let it report in default.
| @@ -111,7 +112,7 @@ private synchronized PBackendService getProxy(TNetworkAddress address) { | |||
| } | |||
|
|
|||
| public Future<PCancelPlanFragmentResult> cancelPlanFragmentAsync( | |||
| TNetworkAddress address, TUniqueId finstId) throws RpcException { | |||
| TNetworkAddress address, TUniqueId finstId, CancelReason cancelReason) throws RpcException { | |||
There was a problem hiding this comment.
This cancelReason is not used?
| for (BackendExecState backendExecState : backendExecStates) { | ||
| TNetworkAddress address = backendExecState.getBackendAddress(); | ||
| LOG.info("cancelRemoteFragments initiated={} done={} hasCanceled={} ip={} port={} fragment instance id={}", | ||
| LOG.info("cancelRemoteFragments initiated={} done={} hasCanceled={} ip={} port={} fragment instance id={}, reason: {}", |
There was a problem hiding this comment.
I think this log can be debug level
imay
reviewed
Jul 17, 2019
gensrc/proto/internal_service.proto
Outdated
| @@ -114,8 +114,16 @@ message PExecPlanFragmentResult { | |||
| required PStatus status = 1; | |||
| }; | |||
|
|
|||
| enum PCancelReason { | |||
morningman
added a commit
to morningman/doris
that referenced
this pull request
Jul 22, 2019
…pache#1489) When query result reach limit, the Coordinator in FE will send a cancel request to BE to cancel the query. And when being cancelled, BE will report query status to FE for debug purpose. But actually it is not necessary and will generate too many logs. So I add a CancelReason to distinguish the difference between 'normally' cancellation and 'internal error' cancellation. if 'normally' cancelled, no status will be reported from BE. When query reach limit, or user cancel it actively, it is being cancelled 'normally'. Otherwise, the query is cancelled due to internal error, which will need a report from BE.
Closed
luwei16
added a commit
to luwei16/incubator-doris
that referenced
this pull request
Apr 7, 2023
…ster (apache#1489) When there are pure compute plan nodes, such as join nodes, in a plan, the original impl. may assign a BE to the node which does not belong to the current cloud cluster. It leads to incomplete isolation of clusters.
SWJTU-ZhangLei
pushed a commit
to SWJTU-ZhangLei/incubator-doris
that referenced
this pull request
Jul 25, 2023
…ster (apache#1489) When there are pure compute plan nodes, such as join nodes, in a plan, the original impl. may assign a BE to the node which does not belong to the current cloud cluster. It leads to incomplete isolation of clusters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When query result reach limit, the Coordinator in FE will send a cancel
request to BE to cancel the query. And when being cancelled, BE will report
query status to FE for debug purpose. But actually it is not necessary
and will generate too many logs.
So I add a CancelReason to distinguish the difference between 'normally'
cancellation and 'internal error' cancellation. if 'normally' cancelled,
no status will be reported to FE.
When query reach limit, or user cancel it actively, it is being cancelled 'normally'.
Otherwise, the query is cancelled due to internal error, which will need
a report from BE.