-
Notifications
You must be signed in to change notification settings - Fork 14.8k
KAFKA-19623: Implement KIP-1147 for console producer/consumer/share-consumer. #20479
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
Conversation
JimmyWang6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShivsundarR Thanks for the patch. I think perhaps console_consumer.py and console_share_consumer.py might need to be changed as well.
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. There are some more changes required too.
--propertyis deprecated and replaced by--formatter-propertyin the two consumer tools- There are a few docs references to the old names
- The docker example references the old names
Yunyung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Partial review only. Please also update the e2e tests and ensure compatibility with older versions.
tools/src/main/java/org/apache/kafka/tools/consumer/ConsoleShareConsumerOptions.java
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/consumer/ConsoleConsumerOptions.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/consumer/ConsoleConsumerOptions.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/consumer/ConsoleConsumerOptions.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/consumer/ConsoleShareConsumerOptionsTest.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/consumer/ConsoleConsumerOptionsTest.java
Outdated
Show resolved
Hide resolved
|
@ShivsundarR Please fix the check style failure. I'll continue with reviewing the PR. |
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to look at docker/test/docker_sanity_test.py.
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor additional comments. Please can you run the docker_sanity_test.py also.
|
Thanks @AndrewJSchofield for the review. I ran the docker-sanity-test locally and it ran successfully. I have attached the results below. Also there was a bug in docker_build_test.py that would not allow running this test using a local image. I had to modify it temporarily to accept --kafka-archive for tests and then it ran. |
|
I ran one of the tests in |
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
chia7712
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShivsundarR Thanks for the patch. Just two small suggestions remain, which could be addressed in a follow-up
| " print.value=true|false\n" + | ||
| " key.separator=<key.separator>\n" + | ||
| " line.separator=<line.separator>\n" + | ||
| " headers.separator=<line.separator>\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: line.separator -> headers.separator
| " print.value=true|false\n" + | ||
| " key.separator=<key.separator>\n" + | ||
| " line.separator=<line.separator>\n" + | ||
| " headers.separator=<line.separator>\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new --formatter-property is not tested in the UT and still uses --property in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have raised follow-up fix here - #20560.
…onsumer. (apache#20479) *What* https://issues.apache.org/jira/browse/KAFKA-19623 - The PR implements KIP-1147 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-1147%3A+Improve+consistency+of+command-line+arguments) for the console tools i.e. `ConsoleProducer`, `ConsoleConsumer` and `ConsoleShareConsumer`. - Currently the previous names for the options are still usable but there will be warning message stating those are deprecated and will be removed in a future version. - I have added unit tests and also manually verified using the console tools that things are working as expected. Reviewers: Andrew Schofield <aschofield@confluent.io>, Jhen-Yung Hsu <jhenyunghsu@gmail.com>, Jimmy Wang <48462172+JimmyWang6@users.noreply.github.com>
…onsumer. (apache#20479) *What* https://issues.apache.org/jira/browse/KAFKA-19623 - The PR implements KIP-1147 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-1147%3A+Improve+consistency+of+command-line+arguments) for the console tools i.e. `ConsoleProducer`, `ConsoleConsumer` and `ConsoleShareConsumer`. - Currently the previous names for the options are still usable but there will be warning message stating those are deprecated and will be removed in a future version. - I have added unit tests and also manually verified using the console tools that things are working as expected. Reviewers: Andrew Schofield <aschofield@confluent.io>, Jhen-Yung Hsu <jhenyunghsu@gmail.com>, Jimmy Wang <48462172+JimmyWang6@users.noreply.github.com>
What
https://issues.apache.org/jira/browse/KAFKA-19623
The PR implements KIP-1147
(https://cwiki.apache.org/confluence/display/KAFKA/KIP-1147%3A+Improve+consistency+of+command-line+arguments)
for the console tools i.e.
ConsoleProducer,ConsoleConsumerandConsoleShareConsumer.Currently the previous names for the options are still usable but
there will be warning message stating those are deprecated and will be
removed in a future version.
I have added unit tests and also manually verified using the console
tools that things are working as expected.
Reviewers: Andrew Schofield aschofield@confluent.io, Jhen-Yung Hsu
jhenyunghsu@gmail.com, Jimmy Wang
48462172+JimmyWang6@users.noreply.github.com