Clone kafka 18894 #31
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
WalkthroughThe change replaces direct usage of ConfigProvider instances with Plugin-wrapped providers across core config resolution and Connect runtime. Constructors, fields, and call sites now accept Map<String, Plugin>. Plugins.newConfigProvider wraps providers with metrics and tags. Tests updated accordingly; a monitorable test provider class and related tests were added. Minor Vagrantfile comment tweak. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin/Worker
participant Plugins as Plugins
participant PluginWrap as Plugin<ConfigProvider>
participant CT as ConfigTransformer
participant CP as ConfigProvider
Admin->>Plugins: newConfigProvider(config, providerName, usage, metrics)
Plugins->>Plugins: Load provider class and configure
Plugins-->>PluginWrap: wrapInstance(provider, metrics, "config.providers", tags)
Admin->>CT: new ConfigTransformer(Map<name, Plugin<ConfigProvider>>)
Admin->>CT: transform(props, configProviders, secretRefs)
CT->>PluginWrap: get()
PluginWrap-->>CT: CP instance
CT->>CP: get(path[, keys])
CP-->>CT: ConfigData
CT-->>Admin: Transformed configs
Admin->>PluginWrap: close (via Utils.closeQuietly)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests
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 of 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
|
Clone kafka 18894TL;DR: Backports Java 8 compatibility changes by removing Java record classes, pattern matching, and other Java 11+ features. Refacto PR SummaryThis PR reverts Java 11+ language features to maintain Java 8 compatibility across the codebase. Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant Developer as Developer
participant BuildSystem as Build System
participant JavaCompiler as Java Compiler
participant ConnectRuntime as Connect Runtime
participant RemoteLogManager as RemoteLogManager
Developer->>BuildSystem: Submit code with Java 8 compatible changes
BuildSystem->>JavaCompiler: Compile with Java 8 target
JavaCompiler-->>BuildSystem: Verify compatibility
Note over ConnectRuntime: Replace records with classes
Note over ConnectRuntime: Replace pattern matching
Note over RemoteLogManager: Move from server to core package
BuildSystem->>ConnectRuntime: Load Connect runtime components
ConnectRuntime->>RemoteLogManager: Initialize remote log functionality
RemoteLogManager-->>ConnectRuntime: Provide backward compatible implementation
ConnectRuntime-->>BuildSystem: All components compatible with Java 8
BuildSystem-->>Developer: Build successful
Testing GuideClick to expand
|
![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 introduces a mechanism to expose metrics from ConfigProvider implementations by wrapping them in a new Plugin<T> class. This allows providers that implement the Monitorable interface to register their own metrics. The changes are well-implemented and consistent across the codebase. The Plugin wrapper is cleanly integrated, and relevant classes like AbstractConfig, ConfigTransformer, and Worker have been updated to use it. The lifecycle management, including closing providers and cleaning up metrics, is handled correctly. The addition of new tests to verify metrics registration and the lifecycle of Monitorable config providers is a great enhancement. Overall, this is a solid contribution that improves the observability of config providers, and I have no suggestions for improvement.
|
/refacto-bot |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Multi-Domain Review: Connect👍 Well Done
📌 Files Processed
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| public class TransformationDoc { | ||
|
|
||
| private record DocInfo(String transformationName, String overview, ConfigDef configDef) { | ||
| private static final class DocInfo { |
There was a problem hiding this comment.
Record to Class Conversion
Converting a Java record to a class loses the automatic generation of equals, hashCode, and toString methods. These methods should be implemented to maintain proper behavior in collections and debugging.
Standards
- Clean Code
- Java Best Practices
- Object Contract
| @@ -22,6 +22,9 @@ | |||
| * Used when a write that requires {@link ConfigBackingStore#claimWritePrivileges() special privileges} fails | |||
| */ | |||
| public class PrivilegedWriteException extends ConnectException { | |||
There was a problem hiding this comment.
Missing Error Handling
The class is missing a constructor that takes only a message parameter. This limits error handling flexibility when detailed cause information isn't available or needed.
Standards
- Complete Exception Hierarchy
- Java Exception Best Practices
| @Override | ||
| public Future<Map<ByteBuffer, ByteBuffer>> get(final Collection<ByteBuffer> keys) { | ||
| ConvertingFutureCallback<Void, Map<ByteBuffer, ByteBuffer>> future = new ConvertingFutureCallback<>() { | ||
| ConvertingFutureCallback<Void, Map<ByteBuffer, ByteBuffer>> future = new ConvertingFutureCallback<Void, Map<ByteBuffer, ByteBuffer>>() { |
There was a problem hiding this comment.
Diamond Operator Missing
The diamond operator is not being used correctly. In older Java versions, type parameters need to be explicitly specified in the constructor when using anonymous classes.
Standards
- Java Backward Compatibility
- Type Safety
Comment on lines
25
to
26
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; |
There was a problem hiding this comment.
Logging Missing Import
Logger imports were added but the class doesn't use them. Missing logger field initialization and usage leads to inconsistent logging across the codebase.
Standards
- Consistent Logging
- Resource Management
| TestPlugins.noOpLoaderSwap()); | ||
| TransformationChain<T, R> realTransformationChainRetriableException = new TransformationChain<>(List.of(stage), toleranceOperator); | ||
| return Mockito.spy(realTransformationChainRetriableException); | ||
| TransformationChain<T, R> realTransformationChainRetriableException = new TransformationChain(List.of(stage), toleranceOperator); |
There was a problem hiding this comment.
Redundant Type Parameters
The code uses raw types in the constructor but maintains generic types in the variable declaration. This inconsistency can lead to type safety issues and compiler warnings.
Standards
- Type Safety
- Consistent Generic Usage
| case EXACTLY_ONCE_FAIL -> throw new ConnectException("oops"); | ||
| default -> null; | ||
| }; | ||
| switch (supportLevel) { |
There was a problem hiding this comment.
Switch Expression Refactoring
Switch expressions are a Java 14+ feature. For compatibility with Java 8, traditional switch statements should be used with explicit return statements.
Standards
- Java 8 Compatibility
- Control Flow Clarity
| case TRANSACTION_BOUNDARIES_NULL -> null; | ||
| default -> ConnectorTransactionBoundaries.UNSUPPORTED; | ||
| }; | ||
| switch (supportLevel) { |
There was a problem hiding this comment.
Second Switch Expression
Another instance of switch expressions which are a Java 14+ feature. For compatibility with Java 8, traditional switch statements should be used with explicit return statements.
Standards
- Java 8 Compatibility
- Control Flow Clarity
| case "awaitStop" -> true; | ||
| default -> null; | ||
| }; | ||
| switch (invocation.getMethod().getName()) { |
There was a problem hiding this comment.
Worker Switch Expression
Another instance of switch expressions which are a Java 14+ feature. For compatibility with Java 8, traditional switch statements should be used with explicit return statements.
Standards
- Java 8 Compatibility
- Control Flow Clarity
| package org.apache.kafka.connect.integration; | ||
|
|
||
| public record StartsAndStops(int starts, int stops) { | ||
| public class StartsAndStops { |
There was a problem hiding this comment.
Java Record Conversion
Java records were introduced in Java 16. For compatibility with Java 8, a regular class with getters should be used instead.
Standards
- Java 8 Compatibility
- API Design
| } | ||
|
|
||
| private record TestDTO(String content) { | ||
| private static class TestDTO { |
There was a problem hiding this comment.
TestDTO Record Conversion
Java records were introduced in Java 16. For compatibility with Java 8, a regular class with getters, equals, and hashCode methods should be used instead.
Standards
- Java 8 Compatibility
- API Design
- Object Contract
| private static String formatAssignment(Map<String, Collection<String>> assignment) { | ||
| StringBuilder result = new StringBuilder(); | ||
| for (String worker : assignment.keySet().stream().sorted().toList()) { | ||
| for (String worker : assignment.keySet().stream().sorted().collect(Collectors.toList())) { |
There was a problem hiding this comment.
Stream toList() Usage
Stream.toList() is a Java 16+ feature. For compatibility with Java 8, collect(Collectors.toList()) should be used instead.
Standards
- Java 8 Compatibility
- Stream API Best Practices
| "simulated-task-producer-" + CONNECTOR_NAME + "-" + i, | ||
| Worker.taskTransactionalId(CLUSTER_GROUP_ID, CONNECTOR_NAME, i) | ||
| )).toList(); | ||
| )).collect(Collectors.toList()); |
There was a problem hiding this comment.
Stream toList() in ExactlyOnce
Stream.toList() is a Java 16+ feature. For compatibility with Java 8, collect(Collectors.toList()) should be used instead.
Standards
- Java 8 Compatibility
- Stream API Best Practices
| .sorted(Comparator.reverseOrder()) | ||
| .map(Path::toFile) | ||
| .toList(); | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Stream toList() in TestPlugins
Stream.toList() is a Java 16+ feature. For compatibility with Java 8, collect(Collectors.toList()) should be used instead.
Standards
- Java 8 Compatibility
- Stream API Best Practices
| .filter(path -> !path.toFile().getName().endsWith(".java")) | ||
| .filter(path -> !removeRuntimeClasses.test(path.toFile().getName())) | ||
| .toList(); | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Stream toList() in TestPlugins
Stream.toList() is a Java 16+ feature. For compatibility with Java 8, collect(Collectors.toList()) should be used instead.
Standards
- Java 8 Compatibility
- Stream API Best Practices
| private String executePut(URI serverUrl, String endpoint, String jsonBody) throws IOException { | ||
| HttpPut request = new HttpPut(endpoint); | ||
| StringEntity entity = new StringEntity(jsonBody, StandardCharsets.UTF_8); | ||
| StringEntity entity = new StringEntity(jsonBody, StandardCharsets.UTF_8.name()); |
There was a problem hiding this comment.
Charset Name String
StringEntity constructor with Charset parameter is not available in older versions of HttpClient. Using the charset name as a string ensures compatibility.
Standards
- API Compatibility
- Backward Compatibility
| ConfigInfos result = herder.validateConnectorConfig(config, s -> null, false); | ||
|
|
||
| assertEquals(ConnectorType.SOURCE, herder.connectorType(config)); | ||
| assertEquals(herder.connectorType(config), ConnectorType.SOURCE); |
There was a problem hiding this comment.
Assertion Parameter Order
The assertEquals parameters are in the wrong order. The expected value should be first, followed by the actual value. This makes error messages clearer when assertions fail.
Standards
- JUnit Best Practices
- Test Readability
|
|
||
| ConfigInfos result = herder.validateConnectorConfig(config, s -> null, false); | ||
| assertEquals(ConnectorType.SOURCE, herder.connectorType(config)); | ||
| assertEquals(herder.connectorType(config), ConnectorType.SOURCE); |
There was a problem hiding this comment.
Assertion Parameter Order
The assertEquals parameters are in the wrong order. The expected value should be first, followed by the actual value. This makes error messages clearer when assertions fail.
Standards
- JUnit Best Practices
- Test Readability
|
|
||
| ConfigInfos result = herder.validateConnectorConfig(config, s -> null, false); | ||
| assertEquals(ConnectorType.SOURCE, herder.connectorType(config)); | ||
| assertEquals(herder.connectorType(config), ConnectorType.SOURCE); |
There was a problem hiding this comment.
Assertion Parameter Order
The assertEquals parameters are in the wrong order. The expected value should be first, followed by the actual value. This makes error messages clearer when assertions fail.
Standards
- JUnit Best Practices
- Test Readability
| .thenReturn(SERIALIZED_RECORD); | ||
| assertEquals(SERIALIZED_KEY, keyConverter.fromConnectData(topic, headers, KEY_SCHEMA, KEY)); | ||
| assertEquals(SERIALIZED_RECORD, valueConverter.fromConnectData(topic, headers, RECORD_SCHEMA, RECORD)); | ||
| assertEquals(keyConverter.fromConnectData(topic, headers, KEY_SCHEMA, KEY), SERIALIZED_KEY); |
There was a problem hiding this comment.
Assertion Parameter Order
The assertEquals parameters are in the wrong order. The expected value should be first, followed by the actual value. This makes error messages clearer when assertions fail.
Standards
- JUnit Best Practices
- Test Readability
| ArgumentCaptor<NotFoundException> exceptionCaptor = ArgumentCaptor.forClass(NotFoundException.class); | ||
| verify(patchCallback).onCompletion(exceptionCaptor.capture(), isNull()); | ||
| assertEquals("Connector " + CONNECTOR_NAME + " not found", exceptionCaptor.getValue().getMessage()); | ||
| assertEquals(exceptionCaptor.getValue().getMessage(), "Connector " + CONNECTOR_NAME + " not found"); |
There was a problem hiding this comment.
Assertion Parameter Order
The assertEquals parameters are in the wrong order. The expected value should be first, followed by the actual value. This makes error messages clearer when assertions fail.
Standards
- JUnit Best Practices
- Test Readability
Comment on lines
235
to
238
| assertEquals(configuration.dlqTopicName(), DLQ_TOPIC); | ||
|
|
||
| configuration = config(singletonMap(SinkConnectorConfig.DLQ_TOPIC_REPLICATION_FACTOR_CONFIG, "7")); | ||
| assertEquals(7, configuration.dlqTopicReplicationFactor()); | ||
| assertEquals(configuration.dlqTopicReplicationFactor(), 7); |
There was a problem hiding this comment.
Assertion Parameter Order
The assertEquals parameters are in the wrong order. The expected value should be first, followed by the actual value. This makes error messages clearer when assertions fail.
Standards
- JUnit Best Practices
- Test Readability
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java (1)
598-641: Trim provider names and close on configure failure
- Trim entries from
config.providersto avoid accidental blanks.- If
configure(...)fails, close the instance before rethrowing. Mirrors existinggetConfiguredInstancefailure cleanup.- for (String provider : configProviders.split(",")) { + for (String raw : configProviders.split(",")) { + String provider = raw.trim(); + if (provider.isEmpty()) continue; String providerClass = providerClassProperty(provider); if (indirectConfigs.containsKey(providerClass)) { String providerClassName = indirectConfigs.get(providerClass); if (classNameFilter.test(providerClassName)) { providerMap.put(provider, providerClassName); } else { throw new ConfigException(providerClassName + " is not allowed. Update System property '" + AUTOMATIC_CONFIG_PROVIDERS_PROPERTY + "' to allow " + providerClassName); } } } // Instantiate Config Providers Map<String, Plugin<ConfigProvider>> configProviderPluginInstances = new HashMap<>(); for (Map.Entry<String, String> entry : providerMap.entrySet()) { try { String prefix = CONFIG_PROVIDERS_CONFIG + "." + entry.getKey() + CONFIG_PROVIDERS_PARAM; Map<String, ?> configProperties = configProviderProperties(prefix, providerConfigProperties); ConfigProvider provider = Utils.newInstance(entry.getValue(), ConfigProvider.class); - provider.configure(configProperties); - Plugin<ConfigProvider> providerPlugin = Plugin.wrapInstance(provider, null, CONFIG_PROVIDERS_CONFIG); + try { + provider.configure(configProperties); + } catch (Throwable t) { + Utils.maybeCloseQuietly(provider, "config provider"); + throw t; + } + Plugin<ConfigProvider> providerPlugin = + Plugin.wrapInstance(provider, null, CONFIG_PROVIDERS_CONFIG); configProviderPluginInstances.put(entry.getKey(), providerPlugin); } catch (ClassNotFoundException e) { log.error("Could not load config provider class {}", entry.getValue(), e); throw new ConfigException(providerClassProperty(entry.getKey()), entry.getValue(), "Could not load config provider class or one of its dependencies"); } }
🧹 Nitpick comments (12)
Vagrantfile (1)
33-33: Optional: Modernize Vagrant defaults (EOL base box, legacy AWS bits).While here, consider follow-ups:
- base_box ubuntu/trusty64 is long EOL; bump to a supported Ubuntu LTS.
- m3.medium/spot price/old AMI likely deprecated; refresh to current types/AMI.
- ifconfig parsing is brittle; prefer ip route/addr or cloud-init metadata for private IP.
Also applies to: 45-47, 115-119, 138-141
clients/src/main/java/org/apache/kafka/common/config/provider/ConfigProvider.java (1)
32-35: Clarify metrics tag semantics in Javadoc.“provider” tag isn’t universally automatic; it’s added when the runtime wraps providers with extra tags (e.g., Connect’s Plugins.newConfigProvider). Direct wraps without extra tags only include “config” and “class”, and no metrics are registered if metrics is null. Suggest tightening wording.
Apply this doc tweak:
- * <p>Implement {@link org.apache.kafka.common.metrics.Monitorable} to enable the config provider to register metrics. - * The following tags are automatically added to all metrics registered: <code>config</code> set to - * <code>config.providers</code>, <code>class</code> set to the ConfigProvider class name, - * and <code>provider</code> set to the provider name. + * <p>Implement {@link org.apache.kafka.common.metrics.Monitorable} to enable the config provider to register metrics. + * When constructed by the Kafka Connect runtime, metrics registered by the provider will be tagged with: + * <code>config</code>=<code>config.providers</code>, <code>class</code>=the provider’s simple class name, + * and <code>provider</code>=the configured provider name. Custom wrappers may omit the <code>provider</code> tag, + * and no metrics are registered if no {@code Metrics} instance is supplied.clients/src/test/java/org/apache/kafka/common/config/ConfigTransformerTest.java (1)
79-82: Add a test for unknown provider behavior.Cover the case where a placeholder references a provider not present in the map; the value should remain unchanged. This guards the null-plugin path.
You can add:
+ @Test + public void testUnknownProviderLeavesValueUnchanged() { + ConfigTransformerResult result = configTransformer.transform(Map.of(MY_KEY, "${unknown:testPath:testKey}")); + assertEquals("${unknown:testPath:testKey}", result.data().get(MY_KEY)); + }clients/src/main/java/org/apache/kafka/common/config/ConfigTransformer.java (2)
67-69: Defensively copy provider map in constructor.Avoid external mutation by copying the incoming map.
- public ConfigTransformer(Map<String, Plugin<ConfigProvider>> configProviderPlugins) { - this.configProviderPlugins = configProviderPlugins; - } + public ConfigTransformer(Map<String, Plugin<ConfigProvider>> configProviderPlugins) { + this.configProviderPlugins = Map.copyOf(configProviderPlugins); + }
98-105: Minor: avoid repeated plugin.get() per path.Cache the provider once per provider iteration for readability and micro-efficiency.
- if (providerPlugin != null && keysByPath != null) { + if (providerPlugin != null && keysByPath != null) { + ConfigProvider provider = providerPlugin.get(); for (Map.Entry<String, Set<String>> pathWithKeys : keysByPath.entrySet()) { String path = pathWithKeys.getKey(); Set<String> keys = new HashSet<>(pathWithKeys.getValue()); - ConfigData configData = providerPlugin.get().get(path, keys); + ConfigData configData = provider.get(path, keys);connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java (1)
2904-2951: Stabilize the test and reduce brittleness
- Start/stop the worker to ensure provider initialization regardless of constructor timing.
- In assertMetrics, match expected tags as a subset instead of exact-equality to avoid future default-tag drift.
@Test public void testMonitorableConfigProvider() { - setup(false); + setup(false); Map<String, String> props = new HashMap<>(this.workerProps); props.put("config.providers", "monitorable,monitorable2"); config = new StandaloneConfig(props); mockKafkaClusterId(); @@ - worker = new Worker(WORKER_ID, new MockTime(), plugins, config, offsetBackingStore, noneConnectorClientConfigOverridePolicy); + worker = new Worker(WORKER_ID, new MockTime(), plugins, config, offsetBackingStore, noneConnectorClientConfigOverridePolicy); + worker.start(); Metrics metrics = worker.metrics().metrics(); @@ - assertMetrics(metrics, + assertMetrics(metrics, 1, expectedTags(WorkerConfig.CONFIG_PROVIDERS_CONFIG, MonitorableConfigProvider.class.getSimpleName(), Map.of("provider", "monitorable"))); @@ - assertMetrics(metrics, + assertMetrics(metrics, 1, expectedTags(WorkerConfig.CONFIG_PROVIDERS_CONFIG, MonitorableConfigProvider.class.getSimpleName(), Map.of("provider", "monitorable2"))); + worker.stop(); } @@ - private void assertMetrics(Metrics metrics, int expected, Map<String, String> expectedTags) { + private void assertMetrics(Metrics metrics, int expected, Map<String, String> expectedTags) { int found = 0; for (MetricName metricName : metrics.metrics().keySet()) { if (metricName.group().equals("plugins")) { Map<String, String> tags = metricName.tags(); - if (expectedTags.equals(tags)) { + boolean matches = expectedTags.entrySet().stream() + .allMatch(e -> e.getValue().equals(tags.get(e.getKey()))); + if (matches) { assertEquals(MonitorableConfigProvider.NAME, metricName.name()); assertEquals(MonitorableConfigProvider.DESCRIPTION, metricName.description()); found++; } } } assertEquals(expected, found); }connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java (1)
48-52: Defensively copy provider mapPrevent accidental external mutation by copying to an unmodifiable map before storing.
- public WorkerConfigTransformer(Worker worker, Map<String, Plugin<ConfigProvider>> configProviderPlugins) { + public WorkerConfigTransformer(Worker worker, Map<String, Plugin<ConfigProvider>> configProviderPlugins) { this.worker = worker; - this.configProviderPlugins = configProviderPlugins; + this.configProviderPlugins = Map.copyOf(configProviderPlugins); this.configTransformer = new ConfigTransformer(configProviderPlugins); }connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java (1)
150-158: Prefer returning empty ConfigDataMinor: returning an empty ConfigData instead of Map.of() on every branch keeps behavior explicit and avoids accidental nulls. Current code is fine though.
clients/src/test/java/org/apache/kafka/common/config/provider/MonitorableConfigProvider.java (1)
41-48: Return empty ConfigData instead of nullAvoid potential NPEs if a test invokes get(...).
- public ConfigData get(String path) { - return null; - } + public ConfigData get(String path) { + return new ConfigData(Map.of()); + } @@ - public ConfigData get(String path, Set<String> keys) { - return null; - } + public ConfigData get(String path, Set<String> keys) { + return new ConfigData(Map.of()); + }connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java (1)
404-413: Tidy up resources (Metrics and plugin)Close Metrics and the returned Plugin to avoid thread/resource leaks in tests.
- Plugin<ConfigProvider> plugin = plugins.newConfigProvider(config, providerName, ClassLoaderUsage.PLUGINS, new Metrics()); - assertInstanceOf(CustomMonitorableConfigProvider.class, plugin.get()); + Metrics metrics = new Metrics(); + try { + Plugin<ConfigProvider> plugin = plugins.newConfigProvider(config, providerName, ClassLoaderUsage.PLUGINS, metrics); + assertInstanceOf(CustomMonitorableConfigProvider.class, plugin.get()); + plugin.close(); + } finally { + metrics.close(); + }connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java (1)
270-286: Close transformer before stopping metricsClose
workerConfigTransformer(which now owns plugin-wrapped providers with plugin metrics) beforemetrics.stop()so plugins can unregister cleanly.globalOffsetBackingStore.stop(); - metrics.stop(); + workerConfigTransformer.close(); + metrics.stop(); log.info("Worker stopped"); workerMetricsGroup.close(); if (connectorStatusMetricsGroup != null) { connectorStatusMetricsGroup.close(); } - workerConfigTransformer.close();clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java (1)
598-622: Optional: warn when provider is listed but its .class is missingToday such providers are silently ignored. Consider logging a warning to aid misconfiguration diagnosis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Vagrantfile(1 hunks)clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java(4 hunks)clients/src/main/java/org/apache/kafka/common/config/ConfigTransformer.java(3 hunks)clients/src/main/java/org/apache/kafka/common/config/provider/ConfigProvider.java(1 hunks)clients/src/test/java/org/apache/kafka/common/config/ConfigTransformerTest.java(5 hunks)clients/src/test/java/org/apache/kafka/common/config/provider/MonitorableConfigProvider.java(1 hunks)connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java(2 hunks)connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java(1 hunks)connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java(3 hunks)connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java(3 hunks)connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java(5 hunks)connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java(5 hunks)connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java (2)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java (1)
Plugins(59-696)
clients/src/test/java/org/apache/kafka/common/config/provider/MonitorableConfigProvider.java (1)
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala (1)
withPluginMetrics(6299-6301)
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java (2)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)clients/src/main/java/org/apache/kafka/common/utils/Utils.java (1)
Utils(93-1700)
clients/src/main/java/org/apache/kafka/common/config/ConfigTransformer.java (1)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)
clients/src/test/java/org/apache/kafka/common/config/ConfigTransformerTest.java (1)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java (1)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java (2)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfig.java (1)
WorkerConfig(58-463)
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java (3)
clients/src/test/java/org/apache/kafka/common/config/provider/MonitorableConfigProvider.java (1)
MonitorableConfigProvider(29-58)clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java (1)
Metrics(70-696)
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java (3)
clients/src/test/java/org/apache/kafka/common/config/provider/MonitorableConfigProvider.java (1)
MonitorableConfigProvider(29-58)clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)clients/src/main/java/org/apache/kafka/common/metrics/Metrics.java (1)
Metrics(70-696)
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java (1)
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java (1)
Plugin(33-92)
🔇 Additional comments (10)
Vagrantfile (1)
20-21: No-op formatting; fine to merge.Blank line + guard comment improve readability; no functional change.
clients/src/test/java/org/apache/kafka/common/config/ConfigTransformerTest.java (1)
49-50: Test setup aligns with new Plugin-wrapped providers.Using Plugin.wrapInstance with null metrics is appropriate for these unit tests. LGTM.
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerTest.java (1)
382-389: Good migration to Plugin-wrapped providersMock returns a Plugin with provider tag; aligns with new Plugins.newConfigProvider signature. LGTM.
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java (1)
100-102: Close semantics LGTMClosing Plugin wrappers via Utils.closeQuietly is correct and ensures both provider and attached PluginMetrics are released.
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java (1)
69-70: Updated setup matches new APIWrapping TestConfigProvider in Plugin and passing to WorkerConfigTransformer is correct. Tests remain valid.
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/PluginsTest.java (2)
391-402: LGTM on Plugin usageSwitch to Plugin wrapper and accessing plugin.get() is correct.
810-816: Nice assertion of configure-before-metricsOverriding withPluginMetrics to assert configured=true is a clean way to validate ordering.
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java (1)
632-639: Clarify/guard null return contractThis method returns null when a provider is listed but
.classis absent. Ensure all call sites handle null to avoid NPEs during transformer initialization. Consider logging a warning here or throwing for stricter config validation.clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java (1)
23-24: LGTM on adopting Plugin wrapper in common configImport and usage align with the new plugin-wrapped provider model.
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java (1)
210-223: Ignore WorkerConfigTransformer null-safety concerns WorkerConfigTransformer never calls get() on the plugin map—it only passes it to ConfigTransformer and iterates values() in close(), both of which handle an empty or missing entry map without NPEs.Likely an incorrect or invalid review comment.
Comment on lines
+550
to
561
| Map<String, Plugin<ConfigProvider>> providerPlugins = instantiateConfigProviders(providerConfigString, configProperties, classNameFilter); | ||
|
|
||
| if (!providers.isEmpty()) { | ||
| ConfigTransformer configTransformer = new ConfigTransformer(providers); | ||
| if (!providerPlugins.isEmpty()) { | ||
| ConfigTransformer configTransformer = new ConfigTransformer(providerPlugins); | ||
| ConfigTransformerResult result = configTransformer.transform(indirectVariables); | ||
| if (!result.data().isEmpty()) { | ||
| resolvedOriginals.putAll(result.data()); | ||
| } | ||
| } | ||
| providers.values().forEach(x -> Utils.closeQuietly(x, "config provider")); | ||
| providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin")); | ||
|
|
||
| return new ResolvingMap<>(resolvedOriginals, originals); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Always close provider plugins even if transform throws
Wrap transform(...) in try/finally so provider plugins are closed on all paths. Prevents leaking provider resources and plugin metrics.
- Map<String, Plugin<ConfigProvider>> providerPlugins = instantiateConfigProviders(providerConfigString, configProperties, classNameFilter);
-
- if (!providerPlugins.isEmpty()) {
- ConfigTransformer configTransformer = new ConfigTransformer(providerPlugins);
- ConfigTransformerResult result = configTransformer.transform(indirectVariables);
- if (!result.data().isEmpty()) {
- resolvedOriginals.putAll(result.data());
- }
- }
- providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin"));
+ Map<String, Plugin<ConfigProvider>> providerPlugins =
+ instantiateConfigProviders(providerConfigString, configProperties, classNameFilter);
+ try {
+ if (!providerPlugins.isEmpty()) {
+ ConfigTransformer configTransformer = new ConfigTransformer(providerPlugins);
+ ConfigTransformerResult result = configTransformer.transform(indirectVariables);
+ if (!result.data().isEmpty()) {
+ resolvedOriginals.putAll(result.data());
+ }
+ }
+ } finally {
+ providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin"));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Map<String, Plugin<ConfigProvider>> providerPlugins = instantiateConfigProviders(providerConfigString, configProperties, classNameFilter); | |
| if (!providers.isEmpty()) { | |
| ConfigTransformer configTransformer = new ConfigTransformer(providers); | |
| if (!providerPlugins.isEmpty()) { | |
| ConfigTransformer configTransformer = new ConfigTransformer(providerPlugins); | |
| ConfigTransformerResult result = configTransformer.transform(indirectVariables); | |
| if (!result.data().isEmpty()) { | |
| resolvedOriginals.putAll(result.data()); | |
| } | |
| } | |
| providers.values().forEach(x -> Utils.closeQuietly(x, "config provider")); | |
| providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin")); | |
| return new ResolvingMap<>(resolvedOriginals, originals); | |
| Map<String, Plugin<ConfigProvider>> providerPlugins = | |
| instantiateConfigProviders(providerConfigString, configProperties, classNameFilter); | |
| try { | |
| if (!providerPlugins.isEmpty()) { | |
| ConfigTransformer configTransformer = new ConfigTransformer(providerPlugins); | |
| ConfigTransformerResult result = configTransformer.transform(indirectVariables); | |
| if (!result.data().isEmpty()) { | |
| resolvedOriginals.putAll(result.data()); | |
| } | |
| } | |
| } finally { | |
| providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin")); | |
| } | |
| return new ResolvingMap<>(resolvedOriginals, originals); |
🤖 Prompt for AI Agents
In clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java
around lines 550 to 561, the call to configTransformer.transform(...) is not
wrapped in a try/finally, so if transform throws the provider plugins may not be
closed; change the code to instantiate the ConfigTransformer and then call
transform inside a try block, applying any result.data() to resolvedOriginals in
the try, and move providerPlugins.values().forEach(x -> Utils.closeQuietly(x,
"config provider plugin")); into a finally block so provider plugins are always
closed even when transform throws.
Comment on lines
270
to
287
| Map<String, String> transform(Map<String, String> props) { | ||
| // transform worker config according to config.providers | ||
| List<String> providerNames = configProviders(); | ||
| Map<String, ConfigProvider> providers = new HashMap<>(); | ||
| Map<String, Plugin<ConfigProvider>> providerPlugins = new HashMap<>(); | ||
| for (String name : providerNames) { | ||
| ConfigProvider configProvider = plugins.newConfigProvider( | ||
| Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider( | ||
| this, | ||
| CONFIG_PROVIDERS_CONFIG + "." + name, | ||
| Plugins.ClassLoaderUsage.PLUGINS | ||
| name, | ||
| Plugins.ClassLoaderUsage.PLUGINS, | ||
| null | ||
| ); | ||
| providers.put(name, configProvider); | ||
| providerPlugins.put(name, configProviderPlugin); | ||
| } | ||
| ConfigTransformer transformer = new ConfigTransformer(providers); | ||
| ConfigTransformer transformer = new ConfigTransformer(providerPlugins); | ||
| Map<String, String> transformed = transformer.transform(props).data(); | ||
| providers.values().forEach(x -> Utils.closeQuietly(x, "config provider")); | ||
| providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin")); | ||
| return transformed; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure plugins are always closed; skip null plugins.
If transform throws, provider plugins aren’t closed, leaking resources/classloaders. Also, newConfigProvider may return null; avoid putting nulls and guard close.
Apply:
- Map<String, Plugin<ConfigProvider>> providerPlugins = new HashMap<>();
- for (String name : providerNames) {
- Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider(
- this,
- name,
- Plugins.ClassLoaderUsage.PLUGINS,
- null
- );
- providerPlugins.put(name, configProviderPlugin);
- }
- ConfigTransformer transformer = new ConfigTransformer(providerPlugins);
- Map<String, String> transformed = transformer.transform(props).data();
- providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider plugin"));
- return transformed;
+ Map<String, Plugin<ConfigProvider>> providerPlugins = new HashMap<>();
+ for (String name : providerNames) {
+ Plugin<ConfigProvider> plugin = plugins.newConfigProvider(
+ this,
+ name,
+ Plugins.ClassLoaderUsage.PLUGINS,
+ null
+ );
+ if (plugin != null) {
+ providerPlugins.put(name, plugin);
+ }
+ }
+ ConfigTransformer transformer = new ConfigTransformer(providerPlugins);
+ try {
+ return transformer.transform(props).data();
+ } finally {
+ providerPlugins.values().forEach(p -> Utils.closeQuietly(p, "config provider plugin"));
+ }If you decide not to filter nulls, please confirm Utils.closeQuietly tolerates nulls.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== newConfigProvider definitions and usage ==="
rg -nR "newConfigProvider" -n -C5 --type java
echo ""
echo "=== Utils.closeQuietly implementations ==="
rg -nR "closeQuietly" -n -C5 --type javaLength of output: 452
🏁 Script executed:
#!/bin/bash
echo "=== Searching for newConfigProvider definitions ==="
rg -n "newConfigProvider" -g '*.java' -C3
echo "=== Searching for Utils.closeQuietly ==="
rg -n "closeQuietly" -g '*.java' -C3Length of output: 168790
🏁 Script executed:
#!/bin/bash
sed -n '600,670p' connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.javaLength of output: 3899
Always null-check and close config provider plugins in a finally block
- Skip any
nullreturned byplugins.newConfigProviderbefore putting intoproviderPlugins. - Wrap the call to
transformer.transform(props)in atry/finallysoUtils.closeQuietlyruns even iftransformthrows. - (Note:
Utils.closeQuietly(null, …)is a no-op, but filtering outnullplugins avoids invalid entries.)
🤖 Prompt for AI Agents
In
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorMakerConfig.java
around lines 270 to 287, the code currently puts possible null config-provider
plugins into the providerPlugins map and only closes plugins after
transformer.transform(props) completes; change it to skip any null returned by
plugins.newConfigProvider before inserting into providerPlugins, call
transformer.transform(props) inside a try block and move the
providerPlugins.values().forEach(x -> Utils.closeQuietly(x, "config provider
plugin")) into a finally block so plugins are closed even if transform throws,
ensuring no null entries are stored and resources are always released.
Comment on lines
646
to
650
| try (LoaderSwap loaderSwap = safeLoaderSwapper().apply(plugin.getClass().getClassLoader())) { | ||
| plugin.configure(configProviderConfig); | ||
| } | ||
| return plugin; | ||
| return Plugin.wrapInstance(plugin, metrics, WorkerConfig.CONFIG_PROVIDERS_CONFIG, Map.of("provider", providerName)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Close provider if configure fails to prevent leaks
If configure(...) throws, the created provider instance is never closed. Close it on failure before wrapping to avoid resource leaks.
- try (LoaderSwap loaderSwap = safeLoaderSwapper().apply(plugin.getClass().getClassLoader())) {
- plugin.configure(configProviderConfig);
- }
- return Plugin.wrapInstance(plugin, metrics, WorkerConfig.CONFIG_PROVIDERS_CONFIG, Map.of("provider", providerName));
+ try (LoaderSwap loaderSwap = safeLoaderSwapper().apply(plugin.getClass().getClassLoader())) {
+ plugin.configure(configProviderConfig);
+ } catch (Throwable t) {
+ // Ensure we don't leak a partially configured provider
+ Utils.maybeCloseQuietly(plugin, "config provider");
+ throw t;
+ }
+ return Plugin.wrapInstance(
+ plugin,
+ metrics,
+ WorkerConfig.CONFIG_PROVIDERS_CONFIG,
+ Map.of("provider", providerName)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try (LoaderSwap loaderSwap = safeLoaderSwapper().apply(plugin.getClass().getClassLoader())) { | |
| plugin.configure(configProviderConfig); | |
| } | |
| return plugin; | |
| return Plugin.wrapInstance(plugin, metrics, WorkerConfig.CONFIG_PROVIDERS_CONFIG, Map.of("provider", providerName)); | |
| } | |
| try (LoaderSwap loaderSwap = safeLoaderSwapper().apply(plugin.getClass().getClassLoader())) { | |
| plugin.configure(configProviderConfig); | |
| } catch (Throwable t) { | |
| // Ensure we don't leak a partially configured provider | |
| Utils.maybeCloseQuietly(plugin, "config provider"); | |
| throw t; | |
| } | |
| return Plugin.wrapInstance( | |
| plugin, | |
| metrics, | |
| WorkerConfig.CONFIG_PROVIDERS_CONFIG, | |
| Map.of("provider", providerName) | |
| ); |
🤖 Prompt for AI Agents
In
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java
around lines 646-650, the code opens a provider instance and calls
plugin.configure(...) but does not close the provider if configure throws,
leaking resources; wrap the configure call in a try/catch so that on any
Throwable you attempt to close the provider (if it implements
AutoCloseable/Closeable cast and call close()), and if close() itself throws add
that exception as suppressed to the original exception before rethrowing; only
call Plugin.wrapInstance(...) after configure completes successfully.
Comment on lines
+214
to
221
| Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider( | ||
| config, | ||
| providerName, | ||
| ClassLoaderUsage.PLUGINS, | ||
| metrics.metrics() | ||
| ); | ||
| providerMap.put(providerName, configProvider); | ||
| providerPluginMap.put(providerName, configProviderPlugin); | ||
| } |
There was a problem hiding this comment.
Guard against null providers to avoid NPEs in transformer
plugins.newConfigProvider(...) can return null. Currently, null is inserted into the map, risking NPEs downstream. Skip nulls and warn.
- Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider(
+ Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider(
config,
providerName,
ClassLoaderUsage.PLUGINS,
metrics.metrics()
);
- providerPluginMap.put(providerName, configProviderPlugin);
+ if (configProviderPlugin != null) {
+ providerPluginMap.put(providerName, configProviderPlugin);
+ } else {
+ log.warn("Skipping config provider '{}' as no class configured under {}.{}.class",
+ providerName, WorkerConfig.CONFIG_PROVIDERS_CONFIG, providerName);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider( | |
| config, | |
| providerName, | |
| ClassLoaderUsage.PLUGINS, | |
| metrics.metrics() | |
| ); | |
| providerMap.put(providerName, configProvider); | |
| providerPluginMap.put(providerName, configProviderPlugin); | |
| } | |
| Plugin<ConfigProvider> configProviderPlugin = plugins.newConfigProvider( | |
| config, | |
| providerName, | |
| ClassLoaderUsage.PLUGINS, | |
| metrics.metrics() | |
| ); | |
| if (configProviderPlugin != null) { | |
| providerPluginMap.put(providerName, configProviderPlugin); | |
| } else { | |
| log.warn("Skipping config provider '{}' as no class configured under {}.{}.class", | |
| providerName, WorkerConfig.CONFIG_PROVIDERS_CONFIG, providerName); | |
| } |
🤖 Prompt for AI Agents
In connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
around lines 214 to 221, plugins.newConfigProvider(...) may return null; change
the logic to check the returned Plugin<ConfigProvider> for null before putting
it into providerPluginMap — if the plugin is null, do not insert it and emit a
warning-level log that includes the providerName (and any contextual config if
useful) so downstream transformers won't encounter an NPE; otherwise proceed to
put the non-null plugin into the map as before.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
New Features
Documentation
Tests
Refactor
Chores