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?
| @@ -97,7 +97,7 @@ class SocketServer( | |||
| 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.
|
| @@ -495,8 +491,7 @@ private[kafka] abstract class Acceptor(val socketServer: SocketServer, | |||
|
|
|||
| val shouldRun = new AtomicBoolean(true) | |||
|
|
|||
| 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!
As described in the JIRA ticket, controlPlaneRequestChannelOpt was
removed 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, since DataPlaneAcceptor
is the only implementation of Acceptor.
For the threadPrefix method, I removed it and used the plain string
"data-plane" directly in Acceptor, while keeping the ThreadPrefix
constant in DataPlaneAcceptor so that other classes can continue to
reference it.