Skip to content

Conversation

jmjoy
Copy link
Member

@jmjoy jmjoy commented Jul 8, 2025

This pull request refactors the Kafka client configuration by introducing a new ClientConfig struct to replace the use of RDKafkaClientConfig directly. The changes improve code clarity, encapsulation, and configurability, particularly by adding support for a custom log level. The most important changes include replacing RDKafkaClientConfig with ClientConfig in multiple files, implementing the new ClientConfig struct, and updating the KafkaReportBuilder to use the new configuration.

Refactoring Kafka Client Configuration:

  • Introduction of ClientConfig struct:

    • Added a new ClientConfig struct in src/reporter/kafka.rs, encapsulating Kafka configuration parameters and log levels. It includes methods for setting parameters, setting log levels, and converting to RDKafkaClientConfig. [1] [2]
  • Replacement of RDKafkaClientConfig with ClientConfig:

    • Updated references to RDKafkaClientConfig in README.md and e2e/src/main.rs to use the new ClientConfig struct. [1] [2] [3]

Updates to KafkaReportBuilder:

  • Refactored KafkaReportBuilder to use ClientConfig:
    • Modified the KafkaReportBuilder struct and its methods to accept and work with ClientConfig instead of RDKafkaClientConfig. This includes changes to the new, new_with_pc, and build methods. [1] [2] [3]

These changes enhance the maintainability and flexibility of the Kafka client configuration by abstracting the configuration details into a dedicated struct.

@jmjoy jmjoy requested a review from Copilot July 8, 2025 11:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces direct use of RDKafkaClientConfig with a new ClientConfig struct to encapsulate Kafka settings (including log level), updates the KafkaReportBuilder API to accept ClientConfig, and revises examples in the README and end-to-end test accordingly.

  • Introduce ClientConfig (with key/value params and optional log level) and mapping into RDKafkaClientConfig
  • Update KafkaReportBuilder::new, new_with_pc, and build to use ClientConfig
  • Refresh imports and usage in README.md and e2e/src/main.rs

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/reporter/kafka.rs Add ClientConfig struct, LogLevel enum, and refactor builder to use it
e2e/src/main.rs Switch example from RDKafkaClientConfig to ClientConfig
README.md Update docs and example code to reference ClientConfig
Comments suppressed due to low confidence (2)

src/reporter/kafka.rs:56

  • [nitpick] The name LogLevel is fairly generic and could conflict in other contexts. Consider renaming it to KafkaLogLevel for clarity and to avoid potential naming collisions.
pub enum LogLevel {

src/reporter/kafka.rs:119

  • There are no existing tests covering ClientConfig behavior. Adding unit tests for set, set_log_level, and to_rdkafka_config would help ensure correct mapping of parameters and log levels.
    fn to_rdkafka_config(&self) -> RDKafkaClientConfig {

@jmjoy jmjoy merged commit 28a4f7b into apache:master Jul 8, 2025
11 checks passed
@jmjoy jmjoy deleted the kafka branch July 8, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants