-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-19625: Consistency of command-line arguments for verifiable producer/consumer #20390
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
Changes from 3 commits
d2d51fe
211e9c5
3863c85
dc5135e
2aeb62a
f1096ef
17f0878
ad7a696
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -249,7 +249,7 @@ def start_cmd(self, node, idx): | |
| if self.repeating_keys is not None: | ||
| cmd += " --repeating-keys %s " % str(self.repeating_keys) | ||
|
|
||
| cmd += " --producer.config %s" % VerifiableProducer.CONFIG_FILE | ||
| cmd += " --command.config %s" % VerifiableProducer.CONFIG_FILE | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| cmd += " 2>> %s | tee -a %s &" % (VerifiableProducer.STDOUT_CAPTURE, VerifiableProducer.STDOUT_CAPTURE) | ||
| return cmd | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -612,7 +612,16 @@ private static ArgumentParser argParser() { | |
| .required(false) | ||
| .type(String.class) | ||
| .metavar("CONFIG_FILE") | ||
| .help("Consumer config properties file (config options shared with command line parameters will be overridden)."); | ||
| .help("(DEPRECATED) Consumer config properties file (config options shared with command line parameters will be overridden)." + | ||
| "This option will be removed in a future version. Use --command-config instead."); | ||
|
|
||
| parser.addArgument("--command-config") | ||
| .action(store()) | ||
| .required(false) | ||
| .type(String.class) | ||
| .metavar("CONFIG-FILE") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The names of the metavars use a mixture of |
||
| .dest("commandConfigFile") | ||
| .help("Consumer config properties file."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we are trying in this KIP to get away from specific names for producer/config config, maybe this could be |
||
|
|
||
| return parser; | ||
| } | ||
|
|
@@ -622,16 +631,27 @@ public static VerifiableConsumer createFromArgs(ArgumentParser parser, String[] | |
|
|
||
| boolean useAutoCommit = res.getBoolean("useAutoCommit"); | ||
| String configFile = res.getString("consumer.config"); | ||
| String commandConfigFile = res.getString("command-config"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you used the destination of |
||
| String brokerHostAndPort = res.getString("bootstrapServer"); | ||
|
|
||
| Properties consumerProps = new Properties(); | ||
| if (configFile != null && commandConfigFile != null) { | ||
| throw new ArgumentParserException("Options --consumer.config and --command-config are mutually exclusive.", parser); | ||
| } | ||
| if (configFile != null) { | ||
| try { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a deprecation message here like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm really sorry—I just woke up, so I made a lot of silly typo errors. :( I’ve already addressed all the comments. Thanks for your review.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, not quite. The deprecation message should be printed if the user specified the old |
||
| consumerProps.putAll(Utils.loadProps(configFile)); | ||
| } catch (IOException e) { | ||
| throw new ArgumentParserException(e.getMessage(), parser); | ||
| } | ||
| } | ||
| if (res.getString(commandConfigFile) != null) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you just use |
||
| try { | ||
| consumerProps.putAll(Utils.loadProps(res.getString(commandConfigFile))); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be |
||
| } catch (IOException e) { | ||
| throw new ArgumentParserException(e.getMessage(), parser); | ||
| } | ||
| } | ||
|
|
||
| GroupProtocol groupProtocol = GroupProtocol.of(res.getString("groupProtocol")); | ||
| consumerProps.put(ConsumerConfig.GROUP_PROTOCOL_CONFIG, groupProtocol.name()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,7 +162,8 @@ private static ArgumentParser argParser() { | |
| .required(false) | ||
| .type(String.class) | ||
| .metavar("CONFIG_FILE") | ||
| .help("Producer config properties file."); | ||
| .help("(DEPRECATED) Producer config properties file. " + | ||
| "This option will be removed in a future version. Use --command-config instead."); | ||
|
|
||
| parser.addArgument("--message-create-time") | ||
| .action(store()) | ||
|
|
@@ -189,6 +190,14 @@ private static ArgumentParser argParser() { | |
| .dest("repeatingKeys") | ||
| .help("If specified, each produced record will have a key starting at 0 increment by 1 up to the number specified (exclusive), then the key is set to 0 again"); | ||
|
|
||
| parser.addArgument("--command-config") | ||
| .action(store()) | ||
| .required(false) | ||
| .type(String.class) | ||
| .metavar("CONFIG-FILE") | ||
|
JimmyWang6 marked this conversation as resolved.
|
||
| .dest("commandConfigFile") | ||
| .help("Producer config properties file."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we are trying in this KIP to get away from specific names for producer/config config, maybe this could be |
||
|
|
||
| return parser; | ||
| } | ||
|
|
||
|
|
@@ -217,6 +226,7 @@ public static VerifiableProducer createFromArgs(ArgumentParser parser, String[] | |
| String topic = res.getString("topic"); | ||
| int throughput = res.getInt("throughput"); | ||
| String configFile = res.getString("producer.config"); | ||
| String commandConfigFile = res.getString("commandConfigFile"); | ||
| Integer valuePrefix = res.getInt("valuePrefix"); | ||
| Long createTime = res.getLong("createTime"); | ||
| Integer repeatingKeys = res.getInt("repeatingKeys"); | ||
|
|
@@ -240,14 +250,25 @@ public static VerifiableProducer createFromArgs(ArgumentParser parser, String[] | |
| producerProps.put(ProducerConfig.ACKS_CONFIG, Integer.toString(res.getInt("acks"))); | ||
| // No producer retries | ||
| producerProps.put(ProducerConfig.RETRIES_CONFIG, "0"); | ||
| if (configFile != null && commandConfigFile != null) { | ||
| throw new ArgumentParserException("Options --producer.config and --command-config are mutually exclusive.", parser); | ||
| } | ||
|
|
||
| if (configFile != null) { | ||
| System.out.println("Option --producer-config has been deprecated and will be removed in a future version. Use --command-config instead."); | ||
|
JimmyWang6 marked this conversation as resolved.
Outdated
|
||
| try { | ||
| producerProps.putAll(loadProps(configFile)); | ||
| } catch (IOException e) { | ||
| throw new ArgumentParserException(e.getMessage(), parser); | ||
| } | ||
| } | ||
|
|
||
| if (commandConfigFile != null) { | ||
| try { | ||
| producerProps.putAll(loadProps(commandConfigFile)); | ||
| } catch (IOException e) { | ||
| throw new ArgumentParserException(e.getMessage(), parser); | ||
| } | ||
| } | ||
| StringSerializer serializer = new StringSerializer(); | ||
| KafkaProducer<String, String> producer = new KafkaProducer<>(producerProps, serializer, serializer); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.