KAFKA-8855; Collect and Expose Client's Name and Version in the Brokers (KIP-511 Part 2)#7749
Conversation
|
retest this please |
dajac
force-pushed
the
kafka-8855-collect-client-name-and-version-2-part2-version2
branch
2 times, most recently
from
fb38365 to
e8964fd
Compare
…rs (KIP-511 Part 2)
dajac
force-pushed
the
kafka-8855-collect-client-name-and-version-2-part2-version2
branch
from
e8964fd to
0339513
Compare
|
retest this please |
|
Hi @dajac , thanks for this PR! It looks good. Do we really need to special-case the SASL case? In the case of SASL, the first ApiVersionsRequest that gets sent is a v0 request which will never have the client information on it anyway, right? It would just be nice if we didn't have to alter the Authenticator interface, but maybe there's something I'm not thinking through. |
Unfortunately, all the clients out there are not equal when it comes to the handling of the ApiVersionsRequest. As you said, with SASL, the java client sends a v0 request which will never have the client information (and sends a second one to provide it). On the other hand, librdkafka always sends only one AVR (latest version) thus we must handle it in the SASL handshake. It is really unfortunate but we don't have the choice if we want to support the whole variety of clients out there. Long term, we should aim for having one mandatory AVR in all cases at the beginning of the connection but this is hard now because the AVR is not mandatory. |
|
retest this please |
1 similar comment
|
retest this please |
|
@dajac : I think we're starting to create too many links between unrelated classes here. We need some separation of concerns. Rather than creating links between all these classes, we should just have a general metrics registry that can be modified from several places. It was kind of complicated to explain to explain what I meant, so I wrote a change and posted it here: dajac#1. Take a look and let me know what you think! |
|
See dajac#1 |
|
@cmccabe Thank you for the suggestion. Interestingly, I prototyped something similar but I thought it was too much just for the ClientInformation. Considering the CipherInformation which has been merged in between, I think that you are right, and that it makes sense to have such registry. I will update the PR based on yours. |
|
@cmccabe I have merged part of your PR in mine and reworked things a little. I have made the following changes:
|
| channel.channelMetadataRegistry.clientInformation) | ||
| val req = new RequestChannel.Request(processor = id, context = context, | ||
| startTimeNanos = nowNanos, memoryPool, receive.payload, requestChannel.metrics) | ||
| if (header.apiKey == ApiKeys.API_VERSIONS) { |
There was a problem hiding this comment.
good to include a short reference to kip-511 in a comment here
|
@cmccabe Thanks for the review. I have addressed your comments. |
|
LGTM. Thanks, @dajac |
|
I’ll update the KIP on Monday to reflect the implementation. |
dajac
deleted the
kafka-8855-collect-client-name-and-version-2-part2-version2
branch
2 participants
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)