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

add SPOOL functionality to KSQL #2789

Merged
merged 8 commits into from May 9, 2019

Conversation

@agavra
Copy link
Contributor

commented May 7, 2019

Description

Add support for SPOOL functionality to KSQL. This allows users to record a CLI session's inputs and outputs, which can be helpful for maintaining and updating demos automatically.

Testing done

ksql> SPOOL 'logs/foo.log';
ksql> PRINT 'foo' FROM BEGINNING;
Format:JSON
{"ROWTIME":1557175521788,"ROWKEY":"one","VALUE":1,"sensor-1":"sensor"}
{"ROWTIME":1557175820268,"ROWKEY":"one","VALUE":1,"sensor-1":"sensor-2"}
{"ROWTIME":1557175917771,"ROWKEY":"one","VALUE":1,"SENSOR":"sensor-2"}
^CTopic printing ceased
ksql> SHOW TOPICS;

 Kafka Topic                 | Registered | Partitions | Partition Replicas | Consumers | ConsumerGroups
---------------------------------------------------------------------------------------------------------
 _schemas                    | false      | 1          | 1                  | 0         | 0
 connect-configs             | false      | 1          | 1                  | 0         | 0
 connect-offsets             | false      | 25         | 1                  | 0         | 0
 connect-statuses            | false      | 5          | 1                  | 0         | 0
 default_ksql_processing_log | true       | 1          | 1                  | 0         | 0
 foo                         | true       | 1          | 1                  | 0         | 0
---------------------------------------------------------------------------------------------------------
ksql> SPOOL OFF;
ksql> SHOW TOPICS;

 Kafka Topic                 | Registered | Partitions | Partition Replicas | Consumers | ConsumerGroups
---------------------------------------------------------------------------------------------------------
 _schemas                    | false      | 1          | 1                  | 0         | 0
 connect-configs             | false      | 1          | 1                  | 0         | 0
 connect-offsets             | false      | 25         | 1                  | 0         | 0
 connect-statuses            | false      | 5          | 1                  | 0         | 0
 default_ksql_processing_log | true       | 1          | 1                  | 0         | 0
 foo                         | true       | 1          | 1                  | 0         | 0
---------------------------------------------------------------------------------------------------------
almog.gavra@Almog-Gavras-MBP15 (10:15:27) ksql: cat logs/foo.log

ksql> PRINT 'foo' FROM BEGINNING;
Format:JSON
{"ROWTIME":1557175521788,"ROWKEY":"one","VALUE":1,"sensor-1":"sensor"}
{"ROWTIME":1557175820268,"ROWKEY":"one","VALUE":1,"sensor-1":"sensor-2"}
{"ROWTIME":1557175917771,"ROWKEY":"one","VALUE":1,"SENSOR":"sensor-2"}
Topic printing ceased

ksql> SHOW TOPICS;

 Kafka Topic                 | Registered | Partitions | Partition Replicas | Consumers | ConsumerGroups
---------------------------------------------------------------------------------------------------------
 _schemas                    | false      | 1          | 1                  | 0         | 0
 connect-configs             | false      | 1          | 1                  | 0         | 0
 connect-offsets             | false      | 25         | 1                  | 0         | 0
 connect-statuses            | false      | 5          | 1                  | 0         | 0
 default_ksql_processing_log | true       | 1          | 1                  | 0         | 0
 foo                         | true       | 1          | 1                  | 0         | 0
---------------------------------------------------------------------------------------------------------

ksql> SPOOL OFF;

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

@agavra agavra requested a review from MichaelDrogalis May 7, 2019

@agavra agavra requested a review from confluentinc/ksql as a code owner May 7, 2019

@MichaelDrogalis

This comment has been minimized.

Copy link

commented May 7, 2019

This is great, thanks for spinning it out so fast, @agavra!

@blueedgenick

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@JimGalasyn
Copy link
Member

left a comment

@agavra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Can you please add a little blurb in https://docs.confluent.io/current/ksql/docs/installation/server-config/config-reference.html? Thanks!

I will take @blueedgenick's approach and update the corresponding docs.

@MichaelDrogalis

This comment has been minimized.

Copy link

commented May 7, 2019

Great suggestion @blueedgenick

@agavra agavra force-pushed the agavra:cli_output branch from 4a404e2 to 600ac41 May 7, 2019

@agavra agavra changed the title tee CLI i/o to a file to allow users to "record" CLI sessions add SPOOL functionality to KSQL May 7, 2019

@agavra agavra added this to the 5.3 milestone May 7, 2019

@agavra agavra requested a review from JimGalasyn May 7, 2019

@JimGalasyn
Copy link
Member

left a comment

LGTM, with one suggestion.

@spena

spena approved these changes May 7, 2019

Copy link
Member

left a comment

LGTM, nice feature @agavra

@vcrfxia

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

The spool command only makes sense when issued through the CLI, right? Why are we adding it to the syntax rather than implementing it as a CliSpecificCommand?

@agavra

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

The spool command only makes sense when issued through the CLI, right? Why are we adding it to the syntax rather than implementing it as a CliSpecificCommand?

@vcrfxia because I didn't know that was a thing! good catch :)

@agavra agavra force-pushed the agavra:cli_output branch from f0a5245 to f63569a May 8, 2019

@vcrfxia
Copy link
Contributor

left a comment

Didn't get a chance to finish looking at the tests yet (have to run to an interview) but this is looking great! Will finish reviewing later today.

if (filePathOrOff.equalsIgnoreCase(OFF)) {
unsetSpool.run();
} else {
setSpool.accept(new File(StringUtil.cleanQuotes(filePathOrOff)));

This comment has been minimized.

Copy link
@vcrfxia

vcrfxia May 9, 2019

Contributor

cleanQuotes only removes single quotes, right? What if the user surrounds the filename in double quotes?

This comment has been minimized.

Copy link
@agavra

agavra May 9, 2019

Author Contributor

I think this is how all string literals work in KSQL, so I'll keep it like that:

ksql> SELECT 'foo' FROM GEO;
foo
^CQuery terminated
ksql> SELECT "foo" FROM GEO;
Column foo cannot be resolved.
Statement: SELECT "foo" FROM GEO;
Caused by: Column foo cannot be resolved.
docs/developer-guide/syntax-reference.rst Outdated Show resolved Hide resolved
@vcrfxia

vcrfxia approved these changes May 9, 2019

Copy link
Contributor

left a comment

Thanks @agavra ! LGTM with a few comments inline across my two reviews.

@agavra agavra merged commit 701886b into confluentinc:master May 9, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@agavra agavra deleted the agavra:cli_output branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.