Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Kafka timeout failures must be caught or made explicitly best-effort before approval.
Pull request overview
Reduces Kafka event publishing delays when brokers are unavailable by defaulting max.block.ms to 2500 ms.
Changes:
- Adds a 2.5-second default producer timeout.
- Preserves explicitly configured values.
File summaries
| File | Summary | Finding |
|---|---|---|
plugins/event-bus/kafka/src/main/java/org/apache/cloudstack/mom/kafka/KafkaEventBus.java |
Applies the default Kafka producer timeout. | Moderate (1 vote): producer timeouts may escape publish() and fail CloudStack operations. |
Review details
Suppressed comments (1)
plugins/event-bus/kafka/src/main/java/org/apache/cloudstack/mom/kafka/KafkaEventBus.java:77
- This shortens the wait but does not make publishing continue after the timeout: when metadata is unavailable,
KafkaProducer.send()throws an uncheckedTimeoutExceptionaftermax.block.ms, whileEventDistributorImplonly catchesEventBusException. Consequently, an unavailable broker can still escapeeventDistributor.publish()and fail the CloudStack operation after 2.5 seconds; catch/wrap the producer failure inpublish()(or otherwise make this failure explicitly best-effort) so the new timeout does not turn broker unavailability into request failures.
if (!props.containsKey("max.block.ms")) {
props.put("max.block.ms", DEFAULT_MAX_BLOCK_MS);
}
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14209 +/- ##
============================================
- Coverage 19.91% 19.91% -0.01%
Complexity 20200 20200
============================================
Files 6373 6373
Lines 577230 577232 +2
Branches 70696 70697 +1
============================================
- Hits 114974 114964 -10
- Misses 449690 449706 +16
+ Partials 12566 12562 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Description
Apache CloudStack offers integrations with several messaging services, one of which is Apache Kafka. During service configuration on the platform, properties are defined that will be passed to the producer, which is responsible for publishing events. Among these properties, the most important are:
bootstrap.servers: List of IP addresses and ports of the brokers (services responsible for receiving messages and serving them to consumers);topic: Message storage unit;key.serializerandvalue.serializer: classes responsible for serializing and deserializing the bytes managed by the brokers.Currently, the only properties managed by Apache CloudStack are
topic,key.serializer, andvalue.serializer. Thetopicproperty has a default value ofcloudstack, and the<key|value>.serializerproperties have the valueorg.apache.kafka.common.serialization.StringSerializer. Other properties can be defined in thekafka.producer.propertiesfile, and these are passed automatically.However, it was noticed that if one of the brokers present in
bootstrap.serveris unavailable, message publishing is blocked until a timeout occurs. This blockage culminates in the exhaustion of the available processing thread pool, which in turn causes several other errors. Upon analyzing the message publishing flow, it was observed that the error with the unavailable broker is caused by a search for Kafka cluster metadata, which occurs during the first publication. This search is not asynchronous and aims to return data such as the number of available brokers, topics, and cluster partitions, which are cached and used in message sending. Because the configured broker is unavailable, the thread is stuck for 60000 ms (1 minute).This value is defined by the max.block.ms setting, which has a default value of 60000 ms, and is responsible for defining the maximum time that the metadata retrieval from the cluster can take and how long the producer should wait for free space in the buffer to write the message.
To prevent errors like this from occurring, a default timeout of 2500 ms (2.5s) has been defined if the property has not been set in the configuration file. This is a palliative change, in order to prevent the pool exhaustion. In the future, I pretend to find another way to prevent the error without having to change the
max.block.msproperty.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Screenshots (if appropriate):
How Has This Been Tested?
I created a Kafka cluster following the official guide (see Kafka Docker image), and validated that even using the new default value of the
max.block.msconfiguration, ACS still could communicate correctly with Kafka.After that, I configured a local Kafka cluster with three nodes, and configured them into ACS. Then, I shut one of the cluster nodes down, and executed an operation that generates an event. When ACS tried to reach the unavailable node, it waited for 2.5 seconds and then move on to the next one.