KAFKA-5549 : Explain that 'client.id' is just used as a prefix within Streams #3544
Conversation
PranavManiar
commented
Jul 18, 2017
PranavManiar
commented
- Added new String CLIENT_ID_DOC in StreamsConfig for explanation
… Streams - Added new String CLIENT_ID_DOC in StreamsConfig for explanation
|
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): |
| @@ -138,6 +138,7 @@ | |||
|
|
|||
| /** {@code client.id} */ | |||
| public static final String CLIENT_ID_CONFIG = CommonClientConfigs.CLIENT_ID_CONFIG; | |||
| private static final String CLIENT_ID_DOC = "An id string, which will be used as a prefix for internal consumer, producer and restore-consumer"; | |||
There was a problem hiding this comment.
Thanks for the patch. I guess, we can be a little bit more precise here. It's a prefix for the client id of the internal clients. Might also be good to add the pattern that is used for the id used for the internal clients.
There was a problem hiding this comment.
Hi mjsax, Thanks for the comment. Does following message sounds good?
"An id string used by internal clients to pass to server when making requests. It will be passed in kafka protocol request header. The format of that header is {api_key:api_version:correlation_id:client_id} "
There was a problem hiding this comment.
What about:
An ID prefix string used for the client IDs of internal consumer, producer and restore-consumer, with pattern '<client.id>-StreamThread-< threadSequenceNumber >-<consumer|producer|restore-consumer>'.";
and
There was a problem hiding this comment.
Agreed.. This description seems perfect.
I don't know how I missed StreamThread in first place :| .. I am just starting up with kafka code.. But this should have been caught in find usage.. Anyways..
I have updated PR with the updated description string. Please check if it looks good.
… Streams - Changed CLIENT_ID_DOC string as per the comment on PR
There was a problem hiding this comment.
Thanks for the patch! LGTM.
Call for second review and merging @guozhangwang @dguy
|
retest this please |
| @@ -146,6 +146,7 @@ | |||
|
|
|||
| /** {@code client.id} */ | |||
| public static final String CLIENT_ID_CONFIG = CommonClientConfigs.CLIENT_ID_CONFIG; | |||
| private static final String CLIENT_ID_DOC = "An ID prefix string used for the client IDs of internal consumer, producer and restore-consumer, with pattern '<client.id>-StreamThread-< threadSequenceNumber >-<consumer|producer|restore-consumer>'."; | |||
There was a problem hiding this comment.
Maybe one more nit (\cc @dguy ): remove the before and after threadSequenceNumber (I added those blanks in my Github comment only to get the right rendering. Otherwise, Github did some weird rendering of my comment. It's not supposed to be part if the message.
Also, can we break this line into multiple... we should try to limit line length to 120 chars. Thx.
There was a problem hiding this comment.
Updated PR. For line breaking character length is bit more than 120 chars, but it's logical one.. I hope that's ok..
There was a problem hiding this comment.
Thx! It's ok. It's just "round about" to keep Github diffs readable.
- Removing additional spaces and breaking long line in mulitple lines
|
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): |
|
Fixing checkstle
|
My bad.. Fixed checkstyle issue now.. Updated PR with the same.. |
|
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 @PranavManiar, merged to trunk |
