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

fix: Allow value delimiter to be specified for datagen #5332

Merged
merged 1 commit into from
May 13, 2020

Conversation

purplefox
Copy link
Contributor

Description

Fixes: #4481

Now value delimiter can be specified when using datagen with delimited format.

Testing done

Manually tested

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

@purplefox purplefox requested a review from a team as a code owner May 11, 2020 16:57
@purplefox purplefox requested a review from big-andy-coates May 11, 2020 16:57
@@ -229,7 +233,7 @@ private static void usage() {
.put("value-format", (builder, arg) -> builder.valueFormat = parseFormat(arg))
// "format" is maintained for backwards compatibility, but should be removed later.
.put("format", (builder, argVal) -> builder.valueFormat = parseFormat(argVal))
.put("value_delimiter",
.put("valueDelimiter",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note: The config is already weird in that it uses both snake case and camel case for arg names :(

So I had to choose one. I chose camel case because more properties seem to use that than snake case.

Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @purplefox.

Am I right in thinking this is only a partitial fix? It doesn't seem to address the second part of the ticket:

Also, if value_delimiter is set to SPACE or TAB the code would use S or T as a delimiter as it stands.

Or did I miss something?

@@ -50,8 +54,13 @@ static DataGenProducer getProducer(
final SerializerFactory<Struct> keySerializerFactory =
keySerializerFactory(keyFormat, ksqlConfig, srClient);

final Map<String, String> formatInfoProperties = new HashMap<>();
if (valueDelimiter != null) {
formatInfoProperties.put(DelimitedFormat.DELIMITER, valueDelimiter.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to check valueFormat is delimited before setting this, and throw an error if its not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack, done

@purplefox purplefox requested a review from big-andy-coates May 12, 2020 08:09
@purplefox
Copy link
Contributor Author

Or did I miss something?

Should be fixed now.

Copy link
Contributor

@big-andy-coates big-andy-coates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @purplefox

LGTM, 'cept 1 nit.

Comment on lines 57 to 60
if (valueDelimiter != null && !(valueFormat instanceof DelimitedFormat)) {
throw new IllegalArgumentException(
"valueDelimiter can only be specified with delimited format");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nite: consider moving this into the if block below that already checks valueDelimiter != null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@purplefox purplefox force-pushed the datagen_value_delimiter branch from 3a75afb to 3de1fd0 Compare May 13, 2020 11:09
@purplefox purplefox merged commit 865e834 into confluentinc:master May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataGen value_delimiter option not wired up.
2 participants