Handle 'RUN SCRIPT' directly in CLI, rather than post it to KSQL. #2331

big-andy-coates · 2019-01-10T14:46:45Z

Description

This is the CLI side of the work for #2179.

Fixed #1031 by deprecating the run script functionality.

The CLI can now handle RUN SCRIPT commands by loading the contents of the script and posting it to KSQL as a request containing multiple lines of SQL, rather than as a special 'run script' command.

i.e. this change means the CLI is no longer reliant on the server's 'run script' functionality. Meaning it can be removed in a later PR. The CLI now handles run script by loading the script content and posting it to the server as a single multi-statement request. This means the rest-server will parse and 'tryExecute' the set of statements, (rather than use the server side run-script functionality which dumbly posts the script contents to the command topic without any validation). If the tryexecute succeeds, then each statement will sequentially be posted to the command topic.

While I was working on a new CLI command I took the opportunity to refactor the interface to decouple it more from the Console.

Instances now take Supplier, Consumer and Runnable interfaces in their constructors. These are wired back to the necessary methods on the Console in the true CLI app.
The console can handle commands with more than one word in their name, e.g. 'run script'.
The console can handle commands that take multiple args.
Dealing with parsing arguments and removing quotes etc is done in one place: in Console.
Cli commands will now ignore any trailing semi-colon, e.g. entering 'version;' will no longer result in an error.

The PR looks to maintain backwards compatibility with existing run script functionality. So you can still type:

ksql>run script '/path/to/my/script';

Though the trailing semi-colon is optional.

With this change, RUN SCRIPT commands entered in the CLI are no longer written to the command topic as a RUN SCRIPT command. Instead, the server will parse the contents of the script, validate, and then write each executable statement to the command topic separately.

There will be follow on PRs to improve the server side handling of multi-statement requests.

Testing done

Manual testing of all existing commands and the new command.

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

- support for CLI commands that take multiple arguments. - Refactor CLI commands - printHelp is now just getHelpMessage - console.flush is handled in one place - the console itself. - CLI commands take suppliers / consumers etc, rather than whole `Console` reference - decoupling the code

hjafarpour

I had a quick pass on the PR and notices that we are not accepting SET and UNSET commands in this version of RUN SCRIPT, so this would not be consistent with the current behavior. In the current behavior, you can have SET or UNSET commands in the script and the engine will honor them. In this PR, the statement fails if we have SET or UNSET commands.
@big-andy-coates What would be your suggestion on handling SET and UNSET in this version of RUN SCRIPT?

ksql-cli/src/main/java/io/confluent/ksql/cli/Cli.java

big-andy-coates · 2019-01-11T16:09:56Z

@hjafarpour Ah.... interesting. I thought the rest api handled SET/UNSET correctly. If that is not the case, then not sure how to handle SET/UNSET. Any suggestions welcome. Otherwise, I'll have a think. I guess we just fix the rest api to handle this right!?!?

So I guess the same is true if someone was to fire off a multi-line statement to the rest api like:

SET 'some.prop'='some.value';
CREATE TABLE t1 ....
UNSET 'some.prop';
CREATE TABLE t2 ...

and add test

big-andy-coates · 2019-01-11T19:26:08Z

As discussed with @hjafarpour, here's the PR to allow the rest-api to support the SET / UNSET commands, (scoped to the single request), needed to match the functionality currently available in the server side run script, which would otherwise be lost with this change.

#2340

hjafarpour

Thanks @big-andy-coates . LGTM with couple of minor comments.
One concern is the conflict between the methods in CmdLineUtil class and our parser. I couldn't find an example that would result in a problem but we should keep an eye on this!

hjafarpour · 2019-01-14T02:14:34Z

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/RunScript.java

+
+class RunScript implements CliSpecificCommand {
+
+  private static final String HELP = "run <path_to_sql_file>:" + System.lineSeparator()


This should be "run script <path_to_sql_file>:".

good catch!

hjafarpour · 2019-01-14T02:19:11Z

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/Help.java

-    console.writer().println();
-    console.writer().println(
+    terminal.println();
+    terminal.println(
        "    1. The line is empty or entirely whitespace. In this"


@big-andy-coates I think this text should have been updated in your previous PR where you made '' for continuous lines optional. Let's update the text here.

Yeah, it should indeed. I've updated.

hjafarpour · 2019-01-14T02:51:17Z

ksql-cli/src/test/java/io/confluent/ksql/cli/console/cmd/RunScriptTest.java

+  @Test
+  public void shouldGetHelp() {
+    assertThat(cmd.getHelpMessage(), is(
+        "run <path_to_sql_file>:" + System.lineSeparator()


Should be run script ....

agavra

I really like the gist of this change.

I haven't looked through everything since it is pretty meaty and (as the resident newbie) I still need to wrap my head on how things fit in to the bigger picture, but here's a first stab at a review (mostly small things)!

Obviously everything is a suggestion, feel free to ignore any comment :)

ksql-cli/src/main/java/io/confluent/ksql/cli/console/Console.java

ksql-cli/src/main/java/io/confluent/ksql/util/CmdLineUtil.java

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/CliSpecificCommand.java

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/History.java

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/CliCmdUtil.java

agavra · 2019-01-16T01:40:22Z

ksql-cli/src/main/java/io/confluent/ksql/util/CmdLineUtil.java

+    }
+  }
+
+  private static final class QuoteRemover {


I think that this and WhitespaceParser are both sufficiently complex to justify dedicated classes (and corresponding unit tests). I still need to go through and review them, but I'll do that with a fresh pair of eyes tomorrow morning :D

IMHO they are implementation details of the methods that use them, and they are tested extensively through those methods.

I'm not saying they've not got bugs ;) . Just that we gain nothing by promoting these to top level classes and testing them independently of the only place they are intended to be used.

Having said that, I'm not particularly a fan of these classes. Raw pattern matching isn't ideal. So if you can suggest a better way to do this... I'm all ears!

(''[^'\s]*)|'(?<esc>([^']|'')*)'|(\S+) This (kinda crazy) Regex seems to do the job pretty well, though at this point I'm not sure if I prefer this "magic" over the original:

String text = "''t0 t1 'quoted string' 'quoted '' string'"; final String NOT_DOUBLE = "''[^'\\s]*"; final String ESCAPED = "([^']|'')*"; final String NOT_WHITESPACE = "\\S+"; String regex = String.format("(%s)|'(?<esc>%s)'|(%s)", NOT_DOUBLE, ESCAPED, NOT_WHITESPACE); Matcher m = Pattern.compile(regex).matcher(text); while (m.find()) { System.out.println(ObjectUtils.defaultIfNull(m.group("esc"), m.group()).replace("''","'")); }

This outputs

't0 t1 quoted string quoted ' string

Interesting!

Does the above regex work for all the test cases? I didn't think it was possible to match quotes with regex in the presence of escaped quotes.

I have not checked - after you push the current code maybe I'll play around with it a little bit, time permitting.

… cli_run_script

big-andy-coates · 2019-01-16T09:53:37Z

Thanks for the review @hjafarpour and welcome @agavra!

rodesai

Took a pass through. Will do one more tomorrow.

rodesai · 2019-01-16T08:41:34Z

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/RunScript.java

+
+  private static String loadScript(final String filePath) {
+    try {
+      return Files.readLines(new File(filePath), StandardCharsets.UTF_8).stream()


Why not just use Files.toString here?

Because its deprecated :)

Mind you, I wasn't intending to use Guava here at all, so you highlighting this has at least sorted that. It now uses Java NIO Files.

rodesai · 2019-01-16T08:46:21Z

ksql-cli/src/main/java/io/confluent/ksql/cli/Cli.java

    terminal
-        .registerCliSpecificCommand(new RemoteServerSpecificCommand(restClient, terminal.writer()));
+        .registerCliSpecificCommand(new RemoteServerSpecificCommand(restClient));


Move this line into registerDefaultCommands?

rodesai · 2019-01-16T08:51:00Z

ksql-cli/src/main/java/io/confluent/ksql/cli/Cli.java

+  }
+
+  @Override
+  public void makeKsqlRequest(final String requestBody) {


nit: rename to something like statementText or statements - requestBody implies the http request body

rodesai · 2019-01-16T09:13:02Z

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/CliCommandRegisterUtil.java


-    console.registerCliSpecificCommand(new Clear(console));
+    console.registerCliSpecificCommand(new Clear(console::clearScreen));


Looks like we've dropped the call to flush() after clearScreen()

Console now explicitly flushes after any command has run. So individual commands don't need to worry about it.

rodesai · 2019-01-16T09:18:00Z

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/History.java

  }

  @Override
-  public void execute(final String commandStrippedLine) {
-    console.printHistory();
-    console.flush();


This flush call gets dropped.

rodesai · 2019-01-16T09:33:40Z

ksql-cli/src/main/java/io/confluent/ksql/util/CmdLineUtil.java

+      pos--;
+    }
+
+    return pos < 0 ? "" : s.substring(0, pos + 1);


you can drop the condition - substring(0, 0) should return ""

Code dropped.

rodesai · 2019-01-16T09:38:59Z

ksql-cli/src/main/java/io/confluent/ksql/util/CmdLineUtil.java

+    private boolean inQuotes;
+    private boolean inWhitespace;
+    private String input;
+    private Builder<String> output;


nit: can we use the full name here (ImmutableList.Builder)? I had to read this a few times before I realized its not building a String.

rodesai · 2019-01-16T09:55:22Z

ksql-cli/src/main/java/io/confluent/ksql/util/CmdLineUtil.java

+   * <ul>
+   * <li>{@code "t0 t1"} becomes {@code ["t0", "t1"]}</li>
+   * <li>{@code "'quoted string'"} becomes {@code ["quoted string"]}</li>
+   * <li>{@code "'quoted'connected'quoted'"} becomes {@code ["quotedconnectedquoted"]}</li>


I think this example should not have the quotes stripped.

If you follow the pattern of how bash shell strips quotes then this should work.

Plus its such an edge case, I don't really want to add special code to not strip here.

OK?

I meant you should fix the example in the comment. The result for 'quoted'connected'quoted' is 'quoted'connected'quoted', right? This function isn't stripping quotes.

Ah, yeah, you're right. I've updated and also added tests covering the javadocs of both the split and strip functions.

agavra

LGTM, though I'll defer the second +1 to Rohan since he's already reviewing and has better context

… cli_run_script

rodesai

LGTM, modulo one inline issue around the docstring for splitByUnquotedWhitespace

… cli_run_script

big-andy-coates · 2019-01-21T13:41:18Z

Thanks all for the reviews!

big-andy-coates added 3 commits December 21, 2018 12:21

Initial new run command

37606f3

Merge branch 'master' into cli_run_script

cdd315d

big-andy-coates requested a review from a team as a code owner January 10, 2019 14:46

hjafarpour reviewed Jan 11, 2019

View reviewed changes

ksql-cli/src/main/java/io/confluent/ksql/cli/Cli.java Outdated Show resolved Hide resolved

big-andy-coates requested a review from a team January 11, 2019 16:08

remove old runscript functionality

1d87461

and add test

big-andy-coates added 2 commits January 11, 2019 19:30

fix test

64c08ca

fix test

196a525

hjafarpour approved these changes Jan 14, 2019

View reviewed changes

big-andy-coates requested a review from a team January 14, 2019 23:55

vcrfxia mentioned this pull request Jan 15, 2019

Make CLI requests wait for last command sequence number #2280

Merged

2 tasks

big-andy-coates and others added 2 commits January 15, 2019 22:01

Hojjat's requested changes.

e64be32

Merge branch 'master' into cli_run_script

07d6db3

agavra reviewed Jan 16, 2019

View reviewed changes

big-andy-coates requested a review from a team January 16, 2019 09:34

big-andy-coates added 2 commits January 16, 2019 09:51

Almog's requested changes.

1dccb8e

Merge branch 'cli_run_script' of github.com:big-andy-coates/ksql into…

3e618ce

… cli_run_script

big-andy-coates and others added 2 commits January 16, 2019 10:02

Almog's requested changes.

0604482

Merge branch 'master' into cli_run_script

6676651

rodesai reviewed Jan 16, 2019

View reviewed changes

Merge branch 'master' into cli_run_script

c2653e7

agavra reviewed Jan 16, 2019

View reviewed changes

big-andy-coates requested a review from rodesai January 18, 2019 15:29

big-andy-coates and others added 3 commits January 18, 2019 15:36

Rohan's requested changes (ish)

a7d7e2c

Merge branch 'cli_run_script' of github.com:big-andy-coates/ksql into…

8cebfc8

… cli_run_script

Merge branch 'master' into cli_run_script

1d3747f

Fix tests.

6f91b84

rodesai approved these changes Jan 18, 2019

View reviewed changes

big-andy-coates and others added 3 commits January 21, 2019 12:34

Merge branch 'master' into cli_run_script

4ea01cd

Merge branch 'cli_run_script' of github.com:big-andy-coates/ksql into…

1de4a61

… cli_run_script

Add tests to cover javadoc comments.

f57a8ab

big-andy-coates merged commit e9cee12 into confluentinc:master Jan 21, 2019

big-andy-coates deleted the cli_run_script branch January 21, 2019 13:47

big-andy-coates mentioned this pull request Feb 12, 2019

Remove 'RUN SCRIPT' functionality from KSQL Server #2179

Closed


		class RunScript implements CliSpecificCommand {

		private static final String HELP = "run <path_to_sql_file>:" + System.lineSeparator()


		console.registerCliSpecificCommand(new Clear(console));
		console.registerCliSpecificCommand(new Clear(console::clearScreen));

Handle 'RUN SCRIPT' directly in CLI, rather than post it to KSQL. #2331

Handle 'RUN SCRIPT' directly in CLI, rather than post it to KSQL. #2331

Conversation

big-andy-coates commented Jan 10, 2019 • edited Loading

Description

Testing done

Reviewer checklist

hjafarpour left a comment

Choose a reason for hiding this comment

big-andy-coates commented Jan 11, 2019 • edited Loading

big-andy-coates commented Jan 11, 2019

hjafarpour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates commented Jan 16, 2019

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra left a comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

big-andy-coates commented Jan 21, 2019

big-andy-coates commented Jan 10, 2019 •

edited

Loading

big-andy-coates commented Jan 11, 2019 •

edited

Loading

big-andy-coates Jan 16, 2019 •

edited

Loading