MINOR: Adjust the timing for creating connect config #20612
Conversation
|
Thanks for the PR. It looks like this also happened for MirrorMaker. |
|
@Yunyung Thank you for pointing this out, MirrorMaker has also been adjusted. |
| try { | ||
| Path pluginPathElement = Paths.get(path).toAbsolutePath(); | ||
| if (pluginPath.isEmpty()) { | ||
| if (path.isEmpty()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think we should also update the
ConnectPluginPathclass to validate thatplugin.pathis 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.
| } | ||
| String pluginPath = properties.getProperty(WorkerConfig.PLUGIN_PATH_CONFIG); | ||
| if (pluginPath != null) { | ||
| if (pluginPath != null && !pluginPath.isEmpty()) { |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
@chia7712 If you think these two changes shouldn't be included in this PR I can make them separately in another PR.
There was a problem hiding this comment.
yes, it would be nice to have a separate PR to revisit those checks
In this PR, we added some
validation checks for the connect config, such as ensuring that
plugin.pathcannot be empty.However, currently, Connect first loads the plugin and then creates the
configuration. Even if
plugin.pathis empty, it still attempts to loadthe 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 [email protected], Chia-Ping Tsai
[email protected]