KAFKA-19606: Fix anomaly of requestHandlerAvgIdleMetric in kraft combined mode #20356
Conversation
There was a problem hiding this comment.
Thanks for the changes @0xffff-zhiyan. Left a couple of comments. Can you attach the screenshots showcasing this as a comment in this thread?
| assertEquals(3, pool.threadPoolSize.get) | ||
|
|
||
| // grow to 5 | ||
| pool.resizeThreadPool(5) | ||
| assertEquals(5, pool.threadPoolSize.get) | ||
|
|
||
| // shrink to 2 | ||
| pool.resizeThreadPool(2) | ||
| assertEquals(2, pool.threadPoolSize.get) |
There was a problem hiding this comment.
I'm not sure this test tests anything new, since the previous implementation would pass it as well.
There was a problem hiding this comment.
This only tests the pool’s resize, including expansion and shrinking, since that’s the part I modified. The previous tests didn’t specifically cover this, so this ensures the pool resizes to the correct size.
| Thread.sleep(200) | ||
| value = sharedMeter.oneMinuteRate() | ||
| } | ||
| assertTrue(value >= 0.0 && value <= 1.05, s"idle percent should be within [0,1], got $value") |
There was a problem hiding this comment.
Shouldn't this assert be [0,1], not [0,1.05]?
There was a problem hiding this comment.
Yes, but I want to allow a small amount of fluctuation to tolerate minor measurement errors.
There was a problem hiding this comment.
Why would there be a measurement error? Ever having a measurement of >1 indicates a bug in our logic determining the denominator.
There was a problem hiding this comment.
Thanks for the improvement @0xffff-zhiyan . I have one high-level suggestion.
| nodeName: String = "broker", | ||
| val isCombinedMode: Boolean = false, |
There was a problem hiding this comment.
This is a very ad-hoc solution which assumes one or two request handler pool(s). To solve this problem, I see two options:
- Keep track of the number of threads in a pool and the number of threads across all of the request handler pools. The metrics would use the global count when computing the average thread idle ratio.
- Have two different metrics for the broker's request handler versus the controller's request handler. This would of course require a KIP.
I am leaning toward options 1 for now. In the future we can always do option 2. which would at a high-level define 3 metrics:
- RequestHandlerAvgIdlePercent which reports the thread idle ratio for all of the request pools
- BrokerRequestHandlerAvgIdlePercent which reports the thread idle ratio for the broker request pool
- ControllerRequestHandlerAvgIdlePercent which reports the thread idle ration for the controller request pool.
JMX metrics RequestHandlerAvgIdlePercent reports a value close to 2 in
combined kraft mode but it's expected to be b/w 0 and 1.
This is an issue with kraft combined mode specifically because both
controller+brokerare using the same Meter object in combinedmode, defined in
RequestThreadIdleMeter#requestThreadIdleMeter, butthe controller and broker are using separate
KafkaRequestHandlerPoolobjects, where each object's
threadPoolSize == KafkaConfig.numIoThreads. This means when calculating idle time, eachpool divides by its own
numIoThreadsvalue before reporting to theshared meter and
Metercalculates the final result by accumulatingall the values reported by all threads. However, since there are
actually
2 × numIoThreadstotal threads contributing to the metric,the denominator should be doubled to get the correct average.
POC in local environment:
Changes:
dataPlaneRequestHandlerPool, we passKafkaConfig.isKRaftCombinedMode; in combined mode the pool adjusts theidle calculation (dividing by
2 * totalHandlerThreads), using thetotal thread count from both pools as the denominator
created/destroyed to ensure the size is up to date
share the
Meterand use separateRequestHandlerPoolhttps://issues.apache.org/jira/browse/KAFKA-19606