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

Make the output of usage of datagen prettier #2651

Conversation

Projects
None yet
4 participants
@danielvdende
Copy link
Contributor

commented Apr 8, 2019

Description

Add some simple newlines to make the usage output for the datagen cli
more readable. The new output looks like this:

[help]
[bootstrap-server=<kafka bootstrap server(s)> (defaults to localhost:9092)]
[quickstart=<quickstart preset> (case-insensitive; one of 'orders', 'users', or 'pageviews')] 
schema=<avro schema file> 
[schemaRegistryUrl=<url for Confluent Schema Registry> (defaults to http://localhost:8081)]
format=<message format> (case-insensitive; one of 'avro', 'json', or 'delimited')
topic=<kafka topic name>  
key=<name of key column> 
[iterations=<number of rows> (defaults to 1,000,000)] 
[maxInterval=<Max time in ms between rows> (defaults to 500)] 
[propertiesFile=<file specifying Kafka client properties>

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

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

@danielvdende danielvdende requested a review from confluentinc/ksql as a code owner Apr 8, 2019

@ConfluentCLABot

This comment has been minimized.

Copy link

commented Apr 8, 2019

It looks like @danielvdende hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@danielvdende

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

[clabot:check]

@ConfluentCLABot

This comment has been minimized.

Copy link

commented Apr 8, 2019

@confluentinc It looks like @danielvdende just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@agavra

agavra approved these changes Apr 8, 2019

Copy link
Contributor

left a comment

Thanks @danielvdende definitely an improvement. If you have the time and desire, though, we might want to leverage something like https://commons.apache.org/proper/commons-cli/apidocs/org/apache/commons/cli/HelpFormatter.html for our CLI usages (and to parse arguments while we're at it).

@agavra agavra requested a review from confluentinc/ksql Apr 8, 2019

@@ -86,20 +86,21 @@ static Properties getProperties(final Arguments arguments) throws IOException {
}

private static void usage() {
String newLine = System.lineSeparator()

This comment has been minimized.

Copy link
@agavra

agavra Apr 8, 2019

Contributor
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.7.1:site (default-site) on project ksql-parent: Error generating maven-checkstyle-plugin:2.17:checkstyle-aggregate report: Failed during checkstyle configuration: 
Exception was thrown while processing /home/****/workspace/confluentinc-pr_ksql_PR-2651/ksql-examples/src/main/java/io/confluent/ksql/datagen/DataGen.java: 
MismatchedTokenException occurred during the analysis of file /home/****/workspace/confluentinc-pr_ksql_PR-2651/ksql-examples/src/main/java/io/confluent/ksql/datagen/DataGen.java. expecting EOF, found '}' -> [Help 1]

Please make sure that this build compiles and add the new help message (copy-paste from output) to the PR description;

This comment has been minimized.

Copy link
@danielvdende

danielvdende Apr 9, 2019

Author Contributor

I've fixed the mistake, and retriggered the CI, but it seems to be timing out. Would expect that to be unrelated to this change though..

This comment has been minimized.

Copy link
@agavra

agavra Apr 9, 2019

Contributor

Weird - I triggered a rebuild and will see if that passes. I agree with you that should be unrelated to your change.

@danielvdende danielvdende force-pushed the danielvdende:feature-prettier-usage-output-datagen branch from 7c04664 to f0e76f0 Apr 9, 2019

@danielvdende

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Hi @agavra, yes definitely. I see this as a quick 'n dirty improvement. I was looking through the code and already considering making this a bigger change, but that would take more time. This change was an easy way to quickly improve readability :-). I'll check out the link you posted and see how far I get. Thanks! 👍

@agavra

agavra approved these changes Apr 9, 2019

@agavra agavra requested a review from confluentinc/ksql Apr 9, 2019

@vcrfxia

vcrfxia approved these changes Apr 9, 2019

Copy link
Contributor

left a comment

Thanks for the contribution @danielvdende ! LGTM. We'll get this merged once we figure out what's going on with the build. Agreed the failures don't seem related to your changes.

@vcrfxia

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Hey @danielvdende , turns out there are Checkstyle issues (in DataGen.java) preventing this PR from building (no idea why the build failures were so cryptic...):

  • Variable 'newLine' should be declared final. (89:12) [FinalLocalVariableCheck]
  • Line is longer than 100 characters (found 104). (98:0) [LineLengthCheck]

Would you mind fixing these when you get a chance? Should be quick. Thanks!

Make the output of usage of datagen prettier
Add some simple newlines to make the usage output for the datagen cli
more readable

@danielvdende danielvdende force-pushed the danielvdende:feature-prettier-usage-output-datagen branch from f0e76f0 to 58d9b38 Apr 10, 2019

@danielvdende

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Hey @vcrfxia, those build failures were definitely cryptic 🙂 I've fixed the errors you mentioned. Thanks for figuring out where the problem was coming from!

@vcrfxia vcrfxia merged commit 70bc505 into confluentinc:master Apr 10, 2019

1 check passed

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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Thanks for the super quick response, @danielvdende -- merged! :)

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