tombentley commented on a change in pull request #10807:
URL: https://github.com/apache/kafka/pull/10807#discussion_r683481734
##########
File path: core/src/main/scala/kafka/server/KafkaApis.scala
##########
@@ -692,6 +692,7 @@ class KafkaApis(val requestChannel: RequestChannel,
fetchRequest.version,
fetchRequest.metadata,
fetchRequest.isFromFollower,
+ s"clientId=${request.context.clientId},
principal=${request.context.principal}",
Review comment:
> It's not ideal that we have to build the string even if we don't use
it.
I couldn't see a nice way to do that. Basing it off the level of the
`FetchSessionCache` logger seemed to break encapsulation, since at the call
site we don't know how this string is going to be used.
>
> In practice, this extra logging is useful if there's a malicious user
forcing sessions to roll or if a user is using a broken client (like Sarama
1.26.0). So I wonder if we really need the clientId. While it's nice to have,
it's a user-controlled field so this could be problematic for large values.
WDYT?
It's a good point that the client controls the `clientId`, and could
potentially choose a long one. The value of logging it here isn't that high, so
I can remove it. If you're making a point about a clientId being maliciously
chosen then that's a bigger problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
