Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove 'RUN SCRIPT' functionality from KSQL Server #2179

Closed
big-andy-coates opened this issue Nov 21, 2018 · 10 comments
Closed

Remove 'RUN SCRIPT' functionality from KSQL Server #2179

big-andy-coates opened this issue Nov 21, 2018 · 10 comments
Assignees

Comments

@big-andy-coates
Copy link
Contributor

The KSQL rest API supports a single request having multiple statements. So it seems unnecessary to have a second 'run script' functionality which is just another way of achieving the same thing, but circumnavigating all the validation and quotes the rest endpoint would otherwise do on the statements.

Run script statements bypass validation and are just written to the command topic, where each server picks up and tries to execute.

We should either completely remove the run script functionality from the REST api, or at the very least mark it as deprecated in the documentation and change the implementation so that the rest api just unpacks it and executes the scripts as though they came through the normal mech.

@miguno
Copy link
Contributor

miguno commented Nov 27, 2018

I know that folks like @ybyzek rely a lot on RUN SCRIPT. What would they do if we removed it? Or are you saying we should remove it from the REST API but keep it in the KSQL CLI (where we would have to unpack the script into separate statements so that the REST API can receive it)?

@ybyzek
Copy link
Contributor

ybyzek commented Dec 3, 2018

Just to clarify one point: automated demos rely on run script because the exec argument was removed. At the end of the day, the use case is to programmatically run commands from a script file. I don't know where this jira leaves us, but it seems like it's going in the opposite direction of the functionality needed?

@big-andy-coates
Copy link
Contributor Author

Hi @miguno / @ybyzek

The plan here is to deprecate the RUN SCRIPT functionality in the server and replace it with the same functionality in CLI. From an end users perspective nothing should have changed, i.e. the same RUN SCRIPT blah.sql; will run from the CLI.

This work has already been done in #2331. It would be great if you could pull down the 5.2 release an ensure everything works to your satisfaction

@ybyzek
Copy link
Contributor

ybyzek commented Feb 13, 2019

From an end users perspective nothing should have changed

This is not true. If user used to send the RUN SCRIPT command to the KSQL server, and now that will be rejected, then from an end user's perspective it has completely broken. ...until user realizes that they need to send the RUN SCRIPT command to the CLI. (The command issued may be the same, but there is huge impact to the user).

Thanks for keeping me in the loop. I definitely want to test this to ensure it works, but I'm slammed this week. Is there time allowance to wait until I test this next week?

cc: @rmoff

@ybyzek
Copy link
Contributor

ybyzek commented Feb 13, 2019

@big-andy-coates , I'm a bit confused.

Today this is how automated demos run a set of KSQL commands:

https://github.com/confluentinc/examples/blob/5.1.0-post/music/start.sh#L23

How is the new functionality going to be different?

@apurvam
Copy link
Contributor

apurvam commented Feb 19, 2019

@ybyzek That should still work, assuming that the CLI and the server are on the same machine. The CLI will continue to take the statements in the sql file passed to run script and then send them as individual statements in a single multiline rest request to the server. As Andy mentioned at the top, this is much more robust as all the existing validations are performed in the latter.

It seems to me that the main change is that now the script has to be on the machine running the CLI. Previously, the script had to be on the machine running the server. Is this true @big-andy-coates ? If so, this still may break some usecases of run script, right?

@ybyzek
Copy link
Contributor

ybyzek commented Feb 19, 2019

@apurvam even today the script is co-located with the CLI, not the KSQL server (see cp-demo example: docker-compose.yml and execution of script).

My understanding of this run script change is there is ZERO impact to CLI (and thus to demos) and only removal from the API/server ("ZERO impact" in terms of how users run it...but obviously there is a change in CLI in sending individual statements now instead of a script of statements)

@apurvam
Copy link
Contributor

apurvam commented Feb 19, 2019

@ybyzek if that’s true then this should be transparent to the CLI.

@big-andy-coates
Copy link
Contributor Author

@ybyzek

How is the new functionality going to be different?

It won't!

The CLI does the heavy lifting for you. It loads the script and fires it at the server as a single multi-line request, which the server processes.

So from your standpoint nothing should change.

@big-andy-coates
Copy link
Contributor Author

After some discussion we've decided to have the rest API flatten any RUN SCRIPT requests so they are processed as though they were sent with the script contents in the main request body.

RUN SCRIPT will still be seen as deprecated functionality in the rest api, (not the CLI, where the functionality will continue). We'll drop it in the next major version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants