[
https://issues.apache.org/jira/browse/IGNITE-10161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16784195#comment-16784195
]
Vladimir Ozerov edited comment on IGNITE-10161 at 3/5/19 7:58 AM:
------------------------------------------------------------------
[~jooger], my comments:
# {{GridQueryKillResponse.cancelReqId}} is better be named as {{reqId}} for the
sake of consistency
# {{RunningQueryManager.queryInProgress}} method looks wrong because we are
still unsure whether our call had any effect. What if it is in progress at the
moment, but not in a progress when actual cancel is called? Moreover, in this
case we need to make two lookups, while only one will be enough. Instead, it is
better to get running query info by ID, check if it is not {{null}}, and then
call {{cancel}} on it directly
# {{CommandProcessor.start}} - as discussed previously, we'd better to complete
futures outside the lock. The reason is that futures may have listeners
attached to them. And we do not want to have the called inside the lock. We
need to collect futures inside the lock, and complete them outside the lock
# {{CommandProcessor.start}} - I'd suggest to use the message "{{Query node has
left the grid: \[nodeId=\]}}". This should be enough.
# {{CommandProcessor.stop}} - in this case the message is wrong. We print ID of
remote node, while it is local node which is stopped. Correct message: "{{Local
node is stopping; \[nodeId==\]}}"
# {{CommandProcessor.onMessage}} - something is wrong with logging here. Why do
we print known messages, but ignore unknown, which is much more severe
situation? It is better to log DEBUG for known message and WARN for unknown one
# {{CommandProcessor.onMessage}} - why {{Throwable}} is swallowed silently?
Most often this is a critical problem and we have a failure handler to react
accordingly. But if we swallow it, abnormal node will remain alive. Moreover,
almost any kind of exception here means that user will have hangs, so I
strongly suggest to avoid any generic error handlers like this. Instead, every
message processing routine must have very careful own handler, which should
swallow with proper logging only expected failures
# What we forgot about is client disconnect handling. We need to fail all
pending futures in case of disconnect, and throw exceptions for all new
{{KILL}} commands until the node is reconnected. Starting point:
{{IgniteH2Indexing.onDisconnected}}
# {{CommandProcessor.processKillQueryCommand}} - please avoid magic numbers.
Version should be a constant with proper name (e.g. {{KILL_COMMAND_SINCE}}).
See how it is done in other places
# {{CommandProcessor.processKillQueryCommand}} - future should be awaited
outside of read lock
was (Author: vozerov):
[~jooger], my comments:
# {{GridQueryKillResponse.cancelReqId}} is better be named as {{reqId}} for the
sake of consistency
# {{RunningQueryManager.queryInProgress}} method looks wrong because we are
still unsure whether our call had any effect. What if it is in progress at the
moment, but not in a progress when actual cancel is called? Moreover, in this
case we need to make two lookups, while only one will be enough. Instead, it is
better to get running query info by ID, check if it is not {{null}}, and then
call {{cancel}} on it directly
# {{CommandProcessor.start}} - as discussed previously, we'd better to complete
futures outside the lock. The reason is that futures may have listeners
attached to them. And we do not want to have the called inside the lock. We
need to collect futures inside the lock, and complete them outside the lock
# {{CommandProcessor.start}} - I'd suggest to use the message "{{Query node has
left the grid: \[nodeId=\]}}". This should be enough.
# {{CommandProcessor.stop}} - in this case the message is wrong. We print ID of
remote node, while it is local node which is stopped. Correct message: "{{Local
node is stopping; \[nodeId==\]}}"
# {{CommandProcessor.onMessage}} - something is wrong with logging here. Why do
we print known messages, but ignore unknown, which is much more severe
situation? It is better to log DEBUG for known message and WARN for unknown one
# {{CommandProcessor.onMessage}} - why {{Throwable}} is swallowed silently?
Most often this is a critical problem and we have a failure handler to react
accordingly. But if we swallow it, abnormal node will remain alive. Moreover,
almost any kind of exception here means that user will have hangs, so I
strongly suggest to avoid any generic error handlers like this. Instead, every
message processing routine must have very careful own handler, which should
swallow with proper logging of only IO failures.
# What we forgot about is client disconnect handling. We need to fail all
pending futures in case of disconnect, and throw exceptions for all new
{{KILL}} commands until the node is reconnected. Starting point:
{{IgniteH2Indexing.onDisconnected}}
# {{CommandProcessor.processKillQueryCommand}} - please avoid magic numbers.
Version should be a constant with proper name (e.g. {{KILL_COMMAND_SINCE}}).
See how it is done in other places
# {{CommandProcessor.processKillQueryCommand}} - future should be awaited
outside of read lock
> Be able to cancel any query by query id
> ---------------------------------------
>
> Key: IGNITE-10161
> URL: https://issues.apache.org/jira/browse/IGNITE-10161
> Project: Ignite
> Issue Type: Task
> Components: sql
> Reporter: Yury Gerzhedovich
> Assignee: Yury Gerzhedovich
> Priority: Major
> Labels: iep-29
> Fix For: 2.8
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> User should be able to cancel any query by query id through SQL command.
> SQL syntax: *KILL QUERY '<_query_id>' \{ASYNC}_*
> _ASYNC_ is optional parameter which return control immediately without
> waiting real cancellation will be done.
> By default, without ASYNC parameter, the request is blocking untill
> cancellation is not done.
> Query should be canceled together with its parts on all nodes.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
