KAFKA-17601: Inter-broker connections do not expose their clientSoftwareName and clientSoftwareVersion tags #17731
Conversation
FrankYang0529
force-pushed
the
KAFKA-17601
branch
from
a719ec2 to
540fcb4
Compare
FrankYang0529
force-pushed
the
KAFKA-17601
branch
from
540fcb4 to
6b87954
Compare
| // KIP-511: ApiVersionsRequest is intercepted here to catch the client software name | ||
| // and version. It is done here to avoid wiring things up to the api layer. | ||
| if (header.apiKey == ApiKeys.API_VERSIONS) { | ||
| val apiVersionsRequest = req.body[ApiVersionsRequest] | ||
| val result = RequestContext.staticParseRequest(header, receive.payload, connectionId, listenerName, channel.principal) | ||
| val apiVersionsRequest = result.request.asInstanceOf[ApiVersionsRequest] | ||
| if (apiVersionsRequest.isValid) { | ||
| channel.channelMetadataRegistry.registerClientInformation(new ClientInformation( | ||
| apiVersionsRequest.data.clientSoftwareName, | ||
| apiVersionsRequest.data.clientSoftwareVersion)) |
There was a problem hiding this comment.
Could we update context#clientInformation directly?
There was a problem hiding this comment.
What I mean is that context#clientInformation gets updated when the request is related to API_VERSIONS.
There was a problem hiding this comment.
Hi @chia7712, thanks for the suggestion. I updated code. Could you take a look again? Thanks.
…areName and clientSoftwareVersion tags Signed-off-by: PoAn Yang <[email protected]>
FrankYang0529
force-pushed
the
KAFKA-17601
branch
from
6b87954 to
fb6c693
Compare
|
We previously did not send api versions for inter broker connections, has that been changed? |
| assertEquals(ClientInformation.UNKNOWN_NAME_OR_VERSION, receivedReq.context.clientInformation.softwareName) | ||
| assertEquals(ClientInformation.UNKNOWN_NAME_OR_VERSION, receivedReq.context.clientInformation.softwareVersion) | ||
| assertEquals(expectedClientSoftwareName, receivedReq.context.clientInformation.softwareName) | ||
| assertEquals(expectedClientSoftwareVersion, receivedReq.context.clientInformation.softwareVersion) |
There was a problem hiding this comment.
The comment above these lines is wrong with this change. I assume there was a reason for the original behavior though - @dajac @rajinisivaram do you recall?
| public final ClientInformation clientInformation; | ||
| // The client information can be updated if the request is ApiVersionRequest, | ||
| // so the client information will not be unknown for ApiVersionRequest. | ||
| public ClientInformation clientInformation; |
There was a problem hiding this comment.
Making this mutable and public seems dangerous. A few things to consider:
- Thread safety.
- Whether we want to allow it to be updated only once (if so, we'd want to have a method that enforces that).
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
When Kafka brokers are connecting to other brokers this information is not properly populated, we see the "unknown" value instead for both
ClientSoftwareNameandClientSoftwareVersion. The reason is that we updatedClientInformationinChannelMetadataRegistryafter we builtRequestConext. We should initializeClientInformationbefore setupRequestContext.Committer Checklist (excluded from commit message)