Clone kafka 18894 #36
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces direct ConfigProvider usage with Plugin-wrapped providers across clients and Connect. Updates constructors, fields, and wiring to pass Map<String, Plugin>. Adjusts provider instantiation to wrap with Plugin (including metrics/tagging), updates transform and cleanup flows accordingly, and adds tests for monitorable providers and metrics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Worker
participant Plugins
participant Plugin as Plugin<ConfigProvider>
participant Provider as ConfigProvider
participant WCT as WorkerConfigTransformer
participant CT as ConfigTransformer
User->>Worker: start / init
Worker->>Plugins: newConfigProvider(config, providerName, classLoaderUsage, metrics)
Plugins->>Plugins: instantiate provider (versioned)
Plugins->>Plugin: wrapInstance(provider, metrics, "config.providers", {"provider": providerName})
Plugins-->>Worker: Plugin<ConfigProvider>
Worker->>WCT: new WorkerConfigTransformer(worker, Map{name->Plugin})
WCT->>CT: new ConfigTransformer(Map{name->Plugin})
User->>WCT: transform(props)
WCT->>CT: transform(propsWithVariables)
CT->>Plugin: get()
Plugin-->>CT: ConfigProvider
CT->>Provider: get(path, keys)
Provider-->>CT: ConfigData(values, ttl)
CT-->>WCT: resolved props, ttls
WCT-->>User: resolved props
Note over Worker,Plugin: Shutdown / cleanup
Worker->>WCT: close()
WCT->>Plugin: close()
Plugin-->>WCT: closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.5)connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @DDShantanuBadmanji, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant refactoring to how Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the instantiation and handling of ConfigProviders by wrapping them in a Plugin class. This is a significant but well-executed change that enables metrics for ConfigProvider implementations that implement the Monitorable interface. The changes are applied consistently across clients and connect-runtime modules, and the tests have been updated accordingly, including new tests for the metrics functionality.
I've identified one issue in AbstractConfig where the provider metric tag is not being added, which is inconsistent with the new documentation and could affect monitoring. I've provided a suggestion to address this. Overall, the refactoring is a good improvement for the codebase.
| ConfigProvider provider = Utils.newInstance(entry.getValue(), ConfigProvider.class); | ||
| provider.configure(configProperties); | ||
| configProviderInstances.put(entry.getKey(), provider); | ||
| Plugin<ConfigProvider> providerPlugin = Plugin.wrapInstance(provider, null, CONFIG_PROVIDERS_CONFIG); |
There was a problem hiding this comment.
The provider metric tag is missing when wrapping the ConfigProvider instance. The updated Javadoc for ConfigProvider states that the provider tag will be added to the metrics. To ensure consistency with how ConfigProvider plugins are created in connect-runtime and to fulfill the documentation, the provider name (entry.getKey()) should be added as a tag.
| Plugin<ConfigProvider> providerPlugin = Plugin.wrapInstance(provider, null, CONFIG_PROVIDERS_CONFIG); | |
| Plugin<ConfigProvider> providerPlugin = Plugin.wrapInstance(provider, null, CONFIG_PROVIDERS_CONFIG, Map.of("provider", entry.getKey())); |
|
The user @DDShantanuBadmanji does not have an active seat allocated for code reviews. Please contact your organization admin to assign a seat or upgrade your subscription. |
2 participants
Delete this text and replace it with a detailed description of your change. The
PR title and body will become the squashed commit message.
If you would like to tag individuals, add some commentary, upload images, or
include other supplemental information that should not be part of the eventual
commit message, please use a separate comment.
If applicable, please include a summary of the testing strategy (including
rationale) for the proposed change. Unit and/or integration tests are expected
for any behavior change and system tests should be considered for larger
changes.
Summary by CodeRabbit