kafka-4866: Kafka console consumer property is ignored #2661
Conversation
Added `print.value` config in ConsoleConsumer to match what the quickstart document says.
|
@ijuma a minor improvement. Please review. Thanks. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks for the PR, can we add a test please? |
Add test in ConsoleConsumerTest#shouldParseValidNewSimpleConsumerValidConfigWithStringOffset for explicitly setting `print.value` to false during issuing ConsoleConsumer command
|
@ijuma sorry for the delay. Added a test item in |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@ijuma Do you have any chances to review the PR? |
|
@hachikuji Could you help review this PR? Thanks |
| if (printValue) { | ||
| write(valueDeserializer, value, lineSeparator) | ||
| } else { | ||
| write(None, "".getBytes(), lineSeparator) |
There was a problem hiding this comment.
Isn't it better to do output.write(lineSeparator) instead?
Addressed Ijuma's comments, outputing a lineSeparator instead of invoking 'write'
|
@ijuma Sounds good. Already addressed your comments. Please review again. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -455,7 +458,11 @@ class DefaultMessageFormatter extends MessageFormatter { | |||
| } | |||
|
|
|||
| if (printKey) write(keyDeserializer, key, keySeparator) | |||
There was a problem hiding this comment.
Maybe we should not output keySeparator is we are not printing values?
Added a helper function `outputSeparator` to cover all the seven situation where printTimestamp, printKey and printValue are set with different values.
|
@ijuma Added a helper function |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@ijuma Did you get any chance to review the new merged code? Thanks in advance. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks for the PR, LGTM. Merged to trunk after some minor tweaks. |
Added
print.valueconfig in ConsoleConsumer to match what the quickstart document says.