feat: add support to terminate all running queries #3944

big-andy-coates · 2019-11-21T17:36:47Z

Description

While investigating an race condition around creating streams I was pained by the process of clearing down my KSQL instance so that I can re-run my test. Before I could drop any sources I had to show queries and then cut / past each query into TERMINATE <id> statements. A right pain!

You can now issue TERMINATE ALL to terminate all running queries.

cc @derekjn @MichaelDrogalis for product review

BREAKING CHANGE: ALL is now a reserved word and can not be used for identifiers without being quoted.

This should help with #2177 too. Not a solution, but helps...

We may choose to add DROP ALL as well. Meaning to reset your KSQL cluster you just need to execute TERMINATE ALL; DROP ALL;

Testing done

usual

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

You can now issue `TERMINATE ALL` to terminate all running queries. BREAKING CHANGE: `ALL` is now a reserved word and can not be used for identifiers without being quoted.

derekjn · 2019-11-21T18:05:50Z

We may choose to add DROP ALL as well.

Sidenote since this isn't part of the PR, but I feel like a good way to achieve this kind of behavior would be to introduce some kind of namespacing abstraction (or whatever we want to call it). Then you'd just DROP CASCADE the whole namespace:

DROP NAMESPACE nsp CASCADE;

This approach requires more intention, and a namespacing capability would be useful in many other ways as well.

ksql-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4

agavra

I'd like to get some alternatives before we do this (i.e. see inline comment or @derekjn's comment about namespaces)

agavra · 2019-11-21T18:20:45Z

ksql-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4

@@ -46,6 +46,7 @@ statement
    | PRINT (identifier| STRING) printClause                                #printTopic
    | (LIST | SHOW) QUERIES EXTENDED?                                       #listQueries
    | TERMINATE QUERY? identifier                                           #terminateQuery
+    | TERMINATE ALL                                                         #terminateQuery


what about changing the semantics of TERMINATE <foo> to use regex instead? That seems much more powerful and we don't need to introduce any new reserved words. Should also be backwards compatible I think

I kind of feel like that could be a slippery slope. e.g. do we introduce regex semantics elsewhere in the grammar? How do users know where they can use regexes and where they can't?

In terms of selectively performing an operation on a number of objects based on a condition, other databases often use catalogs. e.g.,

SELECT terminate(query_id) FROM query_catalog WHERE <condition>

Which I think is a great pattern but obviously not possible for us.

Personally my vote here would be to completely do away with TERMINATE and just make DROP drop the object and its underlying query. It doesn't seem like TERMINATE is useful for users, because as far as I know a terminated query can't be restarted anyways. So it seems to serve as a workaround for the problem of not being able to DROP CASCADE a stream or table.

The slippery slope is fair... that's why I'd like to see these things discussed before adding it 😂

In terms of selectively performing an operation on a number of objects based on a condition, other databases often use catalogs. e.g.,

SELECT terminate(query_id) FROM query_catalog WHERE <condition>

Which I think is a great pattern but obviously not possible for us.

(off topic) The wheels in my mind are turning 😈 I think we should expose a query_catalog as a KSQL table! It's basically a materialization on the command topic. I'm about to create a ticket for this because I think we need to change the command topic to have table semantics anyway.

Proper catalogs would be amazing and would open up all kinds of other operational possibilities. Catalogs also really empower tools, automation etc.

unresolving so that this thread is more visible when people refer back to it.

derekjn · 2019-11-21T19:12:49Z

@big-andy-coates some tangential topics obviously emerged in the PR discussion, but just to be clear I'm fine with introducing TERMINATE ALL for the time being.

big-andy-coates · 2019-11-22T09:30:05Z

Awesome discussion. Agree with all that has been said.

Given we're in a period of flux anyway, how do people (mainly @agavra as he's blocked the PR), feel about merging this for now. (As its better than what we have). Then work on the catalog stuff, (which will likely take time)?

I think there are pros and cons to merging / not merging. Not arguing strongly either way, just weakly in favour of merging as I find it useful to have this functionality, so likely others will to until something better comes along. It's also a trivial change.

agavra · 2019-11-22T16:40:52Z

Yeah I suppose that's fine - I just wanted to see if other people had more alternatives before we merged it; I'm just worried that if we merge it we may never get rid of it 😉 . I'll give this a full review today.

just wanted to hold up a discussion

agavra

What do you think about having TERMINATE ALL be shorthand for distributing individual TERMINATE <query> for all queries? See my comment below (#3944 (comment)).

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/CommandIdAssigner.java

...rc/test/java/io/confluent/ksql/rest/server/computation/InteractiveStatementExecutorTest.java

ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

big-andy-coates · 2019-11-23T07:40:47Z

Created #3966 to track namespaces and #3967 to track dropping terminate all together

Conflicting files ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/InteractiveStatementExecutorTest.java

As per discussion confluentinc#3944 (comment) This PR switching the `TERMINATE ALL` functionality to write a `TERMINATE <querId>` command to the command topic for each active query, rather than a single `TERMINATE ALL` command. The reasoning behind this is that it we _want_ the command topic to be key compacted, and such a design works better with this plan.

feat: add support to terminate all running queries

f60d6c5

You can now issue `TERMINATE ALL` to terminate all running queries. BREAKING CHANGE: `ALL` is now a reserved word and can not be used for identifiers without being quoted.

big-andy-coates requested a review from a team as a code owner November 21, 2019 17:36

big-andy-coates requested review from derekjn and MichaelDrogalis November 21, 2019 17:37

derekjn reviewed Nov 21, 2019

View reviewed changes

ksql-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4 Outdated Show resolved Hide resolved

agavra previously requested changes Nov 21, 2019

View reviewed changes

agavra mentioned this pull request Nov 21, 2019

Breaking Change: Model the Command Topic as a Table #3945

Open

derekjn approved these changes Nov 21, 2019

View reviewed changes

big-andy-coates requested a review from agavra November 22, 2019 09:30

agavra reviewed Nov 22, 2019

View reviewed changes

agavra requested a review from a team November 22, 2019 17:37

big-andy-coates mentioned this pull request Nov 23, 2019

Introduce the concept of a namespace #3966

Closed

big-andy-coates added 2 commits November 23, 2019 07:49

chore: merge from master

e1eff9e

Conflicting files ksql-rest-app/src/test/java/io/confluent/ksql/rest/server/computation/InteractiveStatementExecutorTest.java

Merge branch 'master' into terminate_all

fccd275

big-andy-coates mentioned this pull request Nov 23, 2019

Consider removing the concept of terminating a query #3967

Open

chore: changes requested by Derek

99c39a1

big-andy-coates requested a review from stevenpyzhang November 23, 2019 08:42

big-andy-coates merged commit abbce84 into confluentinc:master Nov 23, 2019

big-andy-coates deleted the terminate_all branch November 23, 2019 15:24

big-andy-coates mentioned this pull request Nov 25, 2019

chore: switch TERMINATE ALL to distribute N TERMINATE queryId stmts #3971

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support to terminate all running queries #3944

feat: add support to terminate all running queries #3944

big-andy-coates commented Nov 21, 2019 •

edited

Loading

derekjn commented Nov 21, 2019

agavra left a comment •

edited

Loading

agavra Nov 21, 2019 •

edited

Loading

derekjn Nov 21, 2019

agavra Nov 21, 2019

derekjn Nov 21, 2019

agavra Nov 21, 2019

big-andy-coates Nov 23, 2019

derekjn commented Nov 21, 2019

big-andy-coates commented Nov 22, 2019 •

edited

Loading

agavra commented Nov 22, 2019

agavra left a comment •

edited

Loading

big-andy-coates commented Nov 23, 2019 •

edited

Loading

feat: add support to terminate all running queries #3944

feat: add support to terminate all running queries #3944

Conversation

big-andy-coates commented Nov 21, 2019 • edited Loading

Description

Testing done

Reviewer checklist

derekjn commented Nov 21, 2019

agavra left a comment • edited Loading

Choose a reason for hiding this comment

agavra Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

derekjn Nov 21, 2019

Choose a reason for hiding this comment

agavra Nov 21, 2019

Choose a reason for hiding this comment

derekjn Nov 21, 2019

Choose a reason for hiding this comment

agavra Nov 21, 2019

Choose a reason for hiding this comment

big-andy-coates Nov 23, 2019

Choose a reason for hiding this comment

derekjn commented Nov 21, 2019

big-andy-coates commented Nov 22, 2019 • edited Loading

agavra commented Nov 22, 2019

agavra left a comment • edited Loading

Choose a reason for hiding this comment

big-andy-coates commented Nov 23, 2019 • edited Loading

big-andy-coates commented Nov 21, 2019 •

edited

Loading

agavra left a comment •

edited

Loading

agavra Nov 21, 2019 •

edited

Loading

big-andy-coates commented Nov 22, 2019 •

edited

Loading

agavra left a comment •

edited

Loading

big-andy-coates commented Nov 23, 2019 •

edited

Loading