chore: switch TERMINATE ALL to distribute N TERMINATE queryId stmts

[big-andy-coates]

Description

As per discussion #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.

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 #")

[stevenpyzhang]

Since this TERMINATE ALL is now enqueueing each terminate query one at a time, it's possible that not all terminate queries are written successfully to the command topic. So a user executing a TERMINATE ALL statement could potentially get an error message back for the terminate query command that failed to enqueue and see that only some of their queries terminated. Is that an acceptable UX? I personally don't think it's that bad, but wanted to raise this point up.

Another point thats kind of orthogonal to this PR, but it'd be nice to be able to at least return partial success results to the user when they submit batched statements.

[big-andy-coates]

I'm wondering if we should both with this, given the main driver was because the current Terminate ALL won't play nice with a key compacted command topic. However, I'm not even sure TERMINATE <queryId> will play well. Currently we post commands for CTAS/CSAS/INSERT INTO, not for the queries themselves.

Maybe the better solution is to remove the terminate functionality completely. Especially given you can't restart a terminated query. For everything except INSERT INTO we can auto-terminate the query if the sink is dropped. Not sure how INSERT INTO would work....

[agavra]

I think the pattern of expanding one statement into multiple is something that can be very useful going forward, and I'm not sure this is the right way to implement it (I've had the urge a few times to rewrite statements in this way). Why not have this done at a higher level (like a statement rewriter)? That way (1) we don't pass the DistributingExecutor into other executors, (2) all the work that is done in RequestHandler doesn't get circumvented and (3) it's easier to extend to other statements that have custom validating that's more complicated.

[agavra]

Currently we post commands for CTAS/CSAS/INSERT INTO, not for the queries themselves.

This is interesting because today they're 1:1 so we post commands both for the DDL and the physical query. I think with @rodesai's changes this will be even more tightly coupled (we're posting exactly the query plan that we want to run). So I still think this makes sense.

[big-andy-coates]

I think the pattern of expanding one statement into multiple is something that can be very useful going forward, and I'm not sure this is the right way to implement it (I've had the urge a few times to rewrite statements in this way). Why not have this done at a higher level (like a statement rewriter)? That way (1) we don't pass the DistributingExecutor into other executors, (2) all the work that is done in RequestHandler doesn't get circumvented and (3) it's easier to extend to other statements that have custom validating that's more complicated.

I'll take a look...

[big-andy-coates]

Since this TERMINATE ALL is now enqueueing each terminate query one at a time, it's possible that not all terminate queries are written successfully to the command topic. So a user executing a TERMINATE ALL statement could potentially get an error message back for the terminate query command that failed to enqueue and see that only some of their queries terminated. Is that an acceptable UX? I personally don't think it's that bad, but wanted to raise this point up.

Another point thats kind of orthogonal to this PR, but it'd be nice to be able to at least return partial success results to the user when they submit batched statements.

Hummm... maybe we can convert TERMINATE ALL into N TERMINATE IF EXIST <queryId>. That should overcome such issues and play well with a key compacted topic (ish)

[big-andy-coates]

Currently we post commands for CTAS/CSAS/INSERT INTO, not for the queries themselves.

This is interesting because today they're 1:1 so we post commands both for the DDL and the physical query. I think with @rodesai's changes this will be even more tightly coupled (we're posting exactly the query plan that we want to run). So I still think this makes sense.

Taking CTAS/CSAS as an example, the command posted to the command topic is keyed out the name of the source (sink) created, not on anything to do with the query. When we terminate we need to keep the source, but drop the query. Not sure how this would work with current design + key compacted topic.

Of course, we could split the CTAS/CSAS into two commands. One to set everything up and a new second one to start the query. Then the terminate would 'delete' the second command, hence stopping the query. (and allow us to support (re)starting the query via statements too if we wanted).

However, I think we first need to decide if we even want terminate statements moving forward. That's a topic for another thread!

Fair to say, for now, this is more likely to be inline with our plans than what's currently there, or we'll be dropping the functionality, so it doesn't matter.

[rodesai]

Of course, we could split the CTAS/CSAS into two commands. One to set everything up and a new second one to start the query. Then the terminate would 'delete' the second command, hence stopping the query. (and allow us to support (re)starting the query via statements too if we wanted).

This is what I was going to suggest. It should be pretty straightforward to do once the execution plan stuff goes in - it already comes back split up this way.

[rodesai]

Code LGTM. It's hard to say how much we'll be able to reuse for the compacted command topic. We should have a proper design for that first IMHO.

[rodesai]

Consider using SqlFormatter here instead of building the statement string.

[big-andy-coates]

As discussed, I don' think this is the right way to solve the TERMINATE ALL, nor the command topic long term, so closing...