MINOR: Adjust the timing for creating connect config #20612

majialoong · 2025-09-29T17:50:40Z

In this PR, we added some
validation checks for the connect config, such as ensuring that
plugin.path cannot be empty.

However, currently, Connect first loads the plugin and then creates the
configuration. Even if plugin.path is empty, it still attempts to load
the plugin first, and then throws an exception when creating the
configuration.

The approach should be to first create a configuration to validate that
the config meet the requirements, and then load the plugin only if the
validation passes. This allows for early detection of problems and
avoids unnecessary plugin loading processes.

Reviewers: Ken Huang s7133700@gmail.com, Chia-Ping Tsai
chia7712@gmail.com

Yunyung · 2025-09-29T18:24:25Z

Thanks for the PR. It looks like this also happened for MirrorMaker.

majialoong · 2025-09-30T01:13:30Z

@Yunyung Thank you for pointing this out, MirrorMaker has also been adjusted.

majialoong · 2025-10-01T01:32:43Z

Hello, @Yunyung @chia7712 @m1a2st , when have time, please review this PR , thanks !

chia7712 · 2025-10-01T08:49:49Z

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java

            try {
                Path pluginPathElement = Paths.get(path).toAbsolutePath();
-                if (pluginPath.isEmpty()) {
+                if (path.isEmpty()) {


Is this a bug fix?

First, in this PR, plugin.path is defined as a non-empty parameter, so this conditional statement should never be executed.

Second, I think this conditional statement was ambiguous. The warning log indicates that the plugin path element is empty, but the actual check is for the entire plugin path, not just the element.

Therefore, we can either remove this conditional statement（since plugin.path cannot be empty）, or modify it to better match the warning log message about an empty plugin path element. I prefer the latter approach.

I think we can remove this check, the path.isEmpty() statement will also never be executed. The List.validator already validates empty values in the list.

kafka/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java

Line 1063 in 423330e

throw new ConfigException("Configuration '" + name + "' values must not be empty.");

I think we should also update the ConnectPluginPath class to validate that plugin.path is not empty. However, this change might require a separate KIP or an update to KIP-1161 to address it.

I think we should also update the ConnectPluginPath class to validate that plugin.path is not empty. However, this change might require a separate KIP or an update to KIP-1161 to address it.

Thanks for your suggestions. I will submit a separate patch to fix the ConnectPluginPath issue; this patch removes the corresponding changes.

chia7712 · 2025-10-01T08:49:54Z

tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java

            }
            String pluginPath = properties.getProperty(WorkerConfig.PLUGIN_PATH_CONFIG);
-            if (pluginPath != null) {
+            if (pluginPath != null && !pluginPath.isEmpty()) {


In this PR, plugin.path is defined to not be empty. Therefore, I think some adjustments are needed here to align with the definition of "non-empty."

@chia7712 If you think these two changes shouldn't be included in this PR I can make them separately in another PR.

yes, it would be nice to have a separate PR to revisit those checks

[In this PR](apache#20334), we added some validation checks for the connect config, such as ensuring that `plugin.path` cannot be empty. However, currently, Connect first loads the plugin and then creates the configuration. Even if `plugin.path` is empty, it still attempts to load the plugin first, and then throws an exception when creating the configuration. The approach should be to first create a configuration to validate that the config meet the requirements, and then load the plugin only if the validation passes. This allows for early detection of problems and avoids unnecessary plugin loading processes. Reviewers: Ken Huang <s7133700@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>

gharris1727 · 2025-11-14T19:23:45Z

@chia7712 @majialoong I don't think this change is correct.

The instantiation of the config must happen after the call to plugins.compareAndSwapWithDelegatingLoader();, otherwise plugin classes which are referenced within the body of the worker config but only present within the plugin.path won't be loadable.

e.g. a custom override policy is installed on the plugin.path and not the classpath, and is no longer able to be found during worker startup, or is initialized with the wrong thread context class loader.

chia7712 · 2025-11-14T19:33:02Z

a custom override policy is installed on the plugin.path and not the classpath, and is no longer able to be found during worker startup, or is initialized with the wrong thread context class loader.

oh, you are right. @majialoong would you mind opening a PR to revert this change? Also, could you please add either a test or a comment to prevent this error from happening again?

majialoong · 2025-11-15T11:03:25Z

@gharris1727 I apologize for causing this error. I only focused on validating the plugin.path parameter and ignored the impact of other parameters. Thank you for pointing this out; I now understand the importance of this order.

I have reverted these changes in another PR #20891 and implemented unit tests to prevent the issue from recurring.

Could you please review it when you have time. Thank you again for pointing out the problem, and I apologize for the changes.

MINOR: Adjust the timing for creating connect config

cd4966f

github-actions bot added triage PRs from the community connect small Small PRs labels Sep 29, 2025

chia7712 added the ci-approved label Sep 29, 2025

Additional adjust to MirrorMaker

650583c

github-actions bot added the mirror-maker-2 label Sep 30, 2025

Adjust the warning log and add non-empty check

87fd1b1

github-actions bot added the tools label Sep 30, 2025

Restore code style

f59ac21

chia7712 reviewed Oct 1, 2025

View reviewed changes

Remove the modify related to ConnectPluginPath

38372c2

github-actions bot removed the triage PRs from the community label Oct 2, 2025

chia7712 approved these changes Oct 6, 2025

View reviewed changes

chia7712 merged commit 24cad50 into apache:trunk Oct 6, 2025
23 of 25 checks passed

majialoong deleted the adjust-connect-config-create-timing branch October 21, 2025 14:09

majialoong mentioned this pull request Nov 6, 2025

MINOR: Add plugin.path config non-empty check in connect-plugin-path.sh #20638

Merged

majialoong mentioned this pull request Nov 15, 2025

MINOR: Revert timing change for creating connect config #20891

Open

MINOR: Adjust the timing for creating connect config #20612

MINOR: Adjust the timing for creating connect config #20612

Uh oh!

Conversation

majialoong commented Sep 29, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yunyung commented Sep 29, 2025

Uh oh!

majialoong commented Sep 30, 2025

Uh oh!

majialoong commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gharris1727 commented Nov 14, 2025

Uh oh!

chia7712 commented Nov 14, 2025

Uh oh!

majialoong commented Nov 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

majialoong commented Sep 29, 2025 •

edited by github-actions bot

Loading