KAFKA-19030: Remove metricNamePrefix from RequestChannel #19374
Conversation
github-actions
bot
added
triage
| private val requestQueueSizeMetricName = RequestQueueSizeMetric | ||
| private val responseQueueSizeMetricName = ResponseQueueSizeMetric |
There was a problem hiding this comment.
We can merge these properties with RequestChannel L47, L48
private val RequestQueueSizeMetric = "RequestQueueSize"
private val ResponseQueueSizeMetric = "ResponseQueueSize"
There was a problem hiding this comment.
@m1a2st Thanks for the comment!
I've directly used RequestQueueSizeMetric and ResponseQueueSizeMetric to eliminate the redundant variable.
There was a problem hiding this comment.
@chia7712 could we also remove DataPlaneAcceptor#ThreadPrefix and DataPlaneAcceptor#MetricPrefix, because the only implementation of Acceptor is DataPlaneAcceptor?
| private val memoryPool = if (config.queuedMaxBytes > 0) new SimpleMemoryPool(config.queuedMaxBytes, config.socketRequestMaxBytes, false, memoryPoolSensor) else MemoryPool.NONE | ||
| // data-plane | ||
| private[network] val dataPlaneAcceptors = new ConcurrentHashMap[EndPoint, DataPlaneAcceptor]() | ||
| val dataPlaneRequestChannel = new RequestChannel(maxQueuedRequests, DataPlaneAcceptor.MetricPrefix, time, apiVersionManager.newRequestMetrics) |
There was a problem hiding this comment.
Could you please try to remove DataPlaneAcceptor.MetricPrefix?
There was a problem hiding this comment.
@chia7712 Yes, I've remove the DataPlaneAcceptor#MetricPrefix.
There was a problem hiding this comment.
Updated: I’m not sure if we should replace the DataPlaneAcceptor#threadPrefix() method with the constant DataPlaneAcceptor#ThreadPrefix, since threadPrefix() is used within the Acceptor interface.
It seems that Acceptor shouldn’t directly rely on constants defined in its child class (DataPlaneAcceptor), as the comment below suggests.
There was a problem hiding this comment.
Acceptor has only one sub class now, so we can remove the abstract methods which was used for multi sub-classes.
Acceptor#threadPrefixDataPlaneAcceptor#ThreadPrefixs"${threadPrefix()}-kafka-network-thread-$nodeId-${endPoint.listenerName}-${endPoint.securityProtocol}-$id"->s"data-plane-kafka-network-thread-$nodeId-${endPoint.listenerName}-${endPoint.securityProtocol}-$id""
There was a problem hiding this comment.
@chia7712 I got it. Thanks for the explanation.
I’ve removed the threadPrefix method from Acceptor, used the plain string "data-plane" directly in Acceptor, and kept the ThreadPrefix constant in DataPlaneAcceptor so other classes can still reference it.
There was a problem hiding this comment.
the impl of KIP-291 is basically gone, maybe we can remove logAndThreadNamePrefix too.
There was a problem hiding this comment.
Okay, thanks for providing the context!
I found that logAndThreadNamePrefix was the only place using DataPlaneAcceptor#ThreadPrefix, so I removed both.
|
|
|
||
| def metricPrefix(): String | ||
| def threadPrefix(): String | ||
| def threadPrefix(): String = DataPlaneAcceptor.ThreadPrefix |
There was a problem hiding this comment.
I'm not sure it's a good idea for a parent class to use constants from its child classes.
There was a problem hiding this comment.
Agreed. I was focused on fixing the previous test case and missed this—thanks for the suggestion!
| import kafka.log.LogManager | ||
| import kafka.log.remote.RemoteLogManager | ||
| import kafka.network.{DataPlaneAcceptor, SocketServer} | ||
| import kafka.network.{SocketServer} |
There was a problem hiding this comment.
import kafka.network.SocketServer
4 participants
As described in the JIRA ticket,
controlPlaneRequestChannelOptwasremoved from KRaft mode, so there's no need to use the metrics prefix
anymore.
This change removes
metricNamePrefixfrom RequestChannel and therelated files.
It also removes
DataPlaneAcceptor#MetricPrefix, sinceDataPlaneAcceptoris the only implementation ofAcceptor.Since the implementation of KIP-291 is essentially removed, we can also
remove
logAndThreadNamePrefixandDataPlaneAcceptor#ThreadPrefix.Reviewers: PoAn Yang payang@apache.org, Ken Huang
s7133700@gmail.com, Chia-Ping Tsai chia7712@gmail.com