KAFKA-10350: add forwarding manager implementation with metrics#9580
KAFKA-10350: add forwarding manager implementation with metrics#9580abbccdda merged 3 commits intoapache:trunkfrom
Conversation
abbccdda
force-pushed
the
forwarding-metrics-KAFKA-10350
branch
from
f0b82d0 to
25d8955
Compare
| BrokerToControllerChannelManagerImpl(metadataCache, time, metrics, | ||
| config, "forwardingChannel", threadNamePrefix) with KafkaMetricsGroup { | ||
|
|
||
| private val forwardingMetricName = "NumRequestsForwardingToControllerPerSec" |
There was a problem hiding this comment.
Wdyt about also adding a "forwarded" boolean to the request log? (in RequestChannel#updateRequestMetrics)
Would this be useful? cc @cmccabe
There was a problem hiding this comment.
Added the logging in RequestChannel.
| metrics: Metrics, | ||
| config: KafkaConfig, | ||
| threadNamePrefix: Option[String] = None) extends | ||
| BrokerToControllerChannelManagerImpl(metadataCache, time, metrics, |
There was a problem hiding this comment.
Should we extend the BrokerToControllerChannel here? It looks like we only ever call forwardRequest, so maybe it's better for ForwardingManager not to extend BrokerToControllerChannel and instead take it as a dependency. Otherwise we are exposing the sendRequest method which we don't really want anyone to use on the forwardingManager instance.
There was a problem hiding this comment.
Actually we do need sendRequest on the forwarding manager interface in the topic creation routing (WIP here.. The forwardRequest is serving the purpose of embedding client request inside an Envelope, while for certain cases broker would like to send direct CreateTopicsRequest on behalf of its own principal instead of the client principal. I think we could debate whether to rename forwardingManager to a different name to also include the possibility of sending direct request, just not have yet come up a better name here.
There was a problem hiding this comment.
Ah, ok 👍 Thanks for the explanation. Another option would be to let KafkaApis have BrokerToControllerChannelManager and ForwardingManager as separate dependencies. However, we already have lots of dependencies there, so I could see us wanting to use one instance for forwarding and regular requests. I don't really have a strong opinion either way, I'm ok leaving it as-is
2 participants
Add metric for forwarding request tracking. Note that the implementation is slightly diverged from the KIP, where we decide to get rid of the client.id tag since most admin clients would only have one inflight forwarding request.
Committer Checklist (excluded from commit message)