Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw IllegalArgumentException when find multiple connector jar for one pluginIdentifier #5551

Merged

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Sep 25, 2023

Purpose of this pull request

Right now, we use prefix match to find the target connector, if we have two plugin jars has the same prefix, then the first one will be used, this may cause problems in some cases. So it's needed to throw an exception if we find two plugin jars for a given plugin.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add UT to test this.

Check list

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_checkIfPluginJarDuplicateInMapping branch 2 times, most recently from d972566 to 05a4698 Compare September 25, 2023 14:56
@Hisoka-X
Copy link
Member

I believe you should add some test case to avoid regression.
image

@Hisoka-X Hisoka-X added this to the 2.3.4 milestone Sep 26, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_checkIfPluginJarDuplicateInMapping branch from 05a4698 to 5a2847a Compare September 26, 2023 11:35
@ruanwenjun
Copy link
Member Author

I believe you should add some test case to avoid regression.
image

Thanks for your review, add UT for this.

final String engineType = pluginIdentifier.getEngineType().toLowerCase();
final String pluginType = pluginIdentifier.getPluginType().toLowerCase();
final String pluginName = pluginIdentifier.getPluginName().toLowerCase();
if (!pluginConfig.hasPath(engineType)) {
if (pluginMappingConfig.isEmpty() || !pluginMappingConfig.hasPath(engineType)) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for add pluginMappingConfig.isEmpty() condition? I think if pluginMappingConfig.isEmpty() is true, the !pluginMappingConfig.hasPath(engineType) is true too?

Comment on lines 68 to 85
@AfterEach
public void after() throws IOException {
for (Path pluginJar : pluginJars) {
Files.deleteIfExists(pluginJar);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should revert seatunnelHome?

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_checkIfPluginJarDuplicateInMapping branch 8 times, most recently from c571ee6 to a31ba69 Compare September 27, 2023 17:31
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_checkIfPluginJarDuplicateInMapping branch from a31ba69 to 71fed74 Compare September 28, 2023 03:34
@ic4y ic4y merged commit 512d982 into apache:dev Sep 28, 2023
4 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_checkIfPluginJarDuplicateInMapping branch September 28, 2023 06:04
gnehil pushed a commit to gnehil/seatunnel that referenced this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants