From 1c18e17b3a2500081377f488db38363470ea38f2 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 23 Feb 2023 12:31:51 -0800 Subject: [PATCH 01/28] KAFKA-15030: Add connect-plugin-path command-line tool Signed-off-by: Greg Harris --- bin/connect-plugin-path.sh | 17 + bin/windows/connect-plugin-path.bat | 17 + build.gradle | 5 + checkstyle/import-control.xml | 2 + config/tools-log4j.properties | 3 + .../connect/runtime/isolation/PluginType.java | 4 + .../runtime/isolation/PluginUtils.java | 9 +- .../connect/runtime/isolation/Plugins.java | 2 +- .../test/plugins/NoManifestConverter.java | 44 ++ .../apache/kafka/tools/ConnectPluginPath.java | 405 ++++++++++++++++++ .../kafka/tools/ConnectPluginPathTest.java | 371 ++++++++++++++++ 11 files changed, 877 insertions(+), 2 deletions(-) create mode 100755 bin/connect-plugin-path.sh create mode 100644 bin/windows/connect-plugin-path.bat create mode 100644 connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java create mode 100644 tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java create mode 100644 tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java diff --git a/bin/connect-plugin-path.sh b/bin/connect-plugin-path.sh new file mode 100755 index 0000000000000..b20ab76759143 --- /dev/null +++ b/bin/connect-plugin-path.sh @@ -0,0 +1,17 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +exec $(dirname $0)/kafka-run-class.sh org.apache.kafka.tools.ConnectPluginPath "$@" diff --git a/bin/windows/connect-plugin-path.bat b/bin/windows/connect-plugin-path.bat new file mode 100644 index 0000000000000..aeb324450cdfe --- /dev/null +++ b/bin/windows/connect-plugin-path.bat @@ -0,0 +1,17 @@ +@echo off +rem Licensed to the Apache Software Foundation (ASF) under one or more +rem contributor license agreements. See the NOTICE file distributed with +rem this work for additional information regarding copyright ownership. +rem The ASF licenses this file to You under the Apache License, Version 2.0 +rem (the "License"); you may not use this file except in compliance with +rem the License. You may obtain a copy of the License at +rem +rem http://www.apache.org/licenses/LICENSE-2.0 +rem +rem Unless required by applicable law or agreed to in writing, software +rem distributed under the License is distributed on an "AS IS" BASIS, +rem WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +rem See the License for the specific language governing permissions and +rem limitations under the License. + +%~dp0kafka-run-class.bat org.apache.kafka.tools.ConnectPluginPath %* \ No newline at end of file diff --git a/build.gradle b/build.gradle index e427061557199..6c8e79ad03617 100644 --- a/build.gradle +++ b/build.gradle @@ -1854,6 +1854,8 @@ project(':tools') { dependencies { implementation project(':clients') implementation project(':server-common') + implementation project(':connect:api') + implementation project(':connect:runtime') implementation project(':log4j-appender') implementation project(':tools:tools-api') implementation libs.argparse4j @@ -1872,6 +1874,9 @@ project(':tools') { testImplementation project(':core').sourceSets.test.output testImplementation project(':server-common') testImplementation project(':server-common').sourceSets.test.output + testImplementation project(':connect:api') + testImplementation project(':connect:runtime') + testImplementation project(':connect:runtime').sourceSets.test.output testImplementation libs.junitJupiter testImplementation libs.mockitoInline // supports mocking static methods, final classes, etc. testImplementation libs.mockitoJunitJupiter // supports MockitoExtension diff --git a/checkstyle/import-control.xml b/checkstyle/import-control.xml index 61e5070d0c1ff..da022a1e1728a 100644 --- a/checkstyle/import-control.xml +++ b/checkstyle/import-control.xml @@ -303,6 +303,8 @@ + + diff --git a/config/tools-log4j.properties b/config/tools-log4j.properties index b19e343265fc3..b669a4e6381b1 100644 --- a/config/tools-log4j.properties +++ b/config/tools-log4j.properties @@ -19,3 +19,6 @@ log4j.appender.stderr=org.apache.log4j.ConsoleAppender log4j.appender.stderr.layout=org.apache.log4j.PatternLayout log4j.appender.stderr.layout.ConversionPattern=[%d] %p %m (%c)%n log4j.appender.stderr.Target=System.err + +# for connect-plugin-path +log4j.logger.org.reflections=ERROR diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginType.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginType.java index 696e14ba8cded..13fbef7430871 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginType.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginType.java @@ -59,6 +59,10 @@ public String simpleName() { return klass.getSimpleName(); } + public String typeName() { + return klass.getName(); + } + @Override public String toString() { return super.toString().toLowerCase(Locale.ROOT); diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index e88bfa1b03d24..2aa1cf8ef7ec4 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -20,6 +20,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.FileNotFoundException; import java.io.IOException; import java.lang.reflect.Modifier; import java.net.MalformedURLException; @@ -196,7 +197,7 @@ public static boolean isClassFile(Path path) { return path.toString().toLowerCase(Locale.ROOT).endsWith(".class"); } - public static List pluginLocations(String pluginPath) { + public static List pluginLocations(String pluginPath, boolean failFast) { if (pluginPath == null) { return Collections.emptyList(); } @@ -205,6 +206,9 @@ public static List pluginLocations(String pluginPath) { for (String path : pluginPathElements) { try { Path pluginPathElement = Paths.get(path).toAbsolutePath(); + if (!Files.exists(pluginPathElement)) { + throw new FileNotFoundException(pluginPathElement.toString()); + } // Currently 'plugin.paths' property is a list of top-level directories // containing plugins if (Files.isDirectory(pluginPathElement)) { @@ -213,6 +217,9 @@ public static List pluginLocations(String pluginPath) { pluginLocations.add(pluginPathElement); } } catch (InvalidPathException | IOException e) { + if (failFast) { + throw new RuntimeException(e); + } log.error("Could not get listing for plugin path: {}. Ignoring.", path, e); } } diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java index 83dec38a6fb22..809c591f1f1a3 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/Plugins.java @@ -63,7 +63,7 @@ public Plugins(Map props) { // VisibleForTesting Plugins(Map props, ClassLoader parent, ClassLoaderFactory factory) { String pluginPath = WorkerConfig.pluginPath(props); - List pluginLocations = PluginUtils.pluginLocations(pluginPath); + List pluginLocations = PluginUtils.pluginLocations(pluginPath, false); delegatingLoader = factory.newDelegatingClassLoader(parent); Set pluginSources = PluginUtils.pluginSources(pluginLocations, delegatingLoader, factory); scanResult = initLoaders(pluginSources); diff --git a/connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java b/connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java new file mode 100644 index 0000000000000..e0f0cbbf5ff70 --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import java.util.Map; +import org.apache.kafka.connect.data.Schema; +import org.apache.kafka.connect.data.SchemaAndValue; +import org.apache.kafka.connect.storage.Converter; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + */ +public class NoManifestConverter implements Converter { + + @Override + public void configure(final Map configs, final boolean isKey) { + } + + @Override + public byte[] fromConnectData(final String topic, final Schema schema, final Object value) { + return new byte[0]; + } + + @Override + public SchemaAndValue toConnectData(final String topic, final byte[] value) { + return null; + } +} diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java new file mode 100644 index 0000000000000..6848a1dd55208 --- /dev/null +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -0,0 +1,405 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.tools; + +import net.sourceforge.argparse4j.ArgumentParsers; +import net.sourceforge.argparse4j.impl.Arguments; +import net.sourceforge.argparse4j.inf.ArgumentGroup; +import net.sourceforge.argparse4j.inf.ArgumentParser; +import net.sourceforge.argparse4j.inf.ArgumentParserException; +import net.sourceforge.argparse4j.inf.Namespace; +import org.apache.kafka.common.utils.Exit; +import org.apache.kafka.common.utils.Utils; +import org.apache.kafka.connect.runtime.WorkerConfig; +import org.apache.kafka.connect.runtime.isolation.ClassLoaderFactory; +import org.apache.kafka.connect.runtime.isolation.DelegatingClassLoader; +import org.apache.kafka.connect.runtime.isolation.PluginDesc; +import org.apache.kafka.connect.runtime.isolation.PluginScanResult; +import org.apache.kafka.connect.runtime.isolation.PluginSource; +import org.apache.kafka.connect.runtime.isolation.PluginType; +import org.apache.kafka.connect.runtime.isolation.PluginUtils; +import org.apache.kafka.connect.runtime.isolation.ReflectionScanner; +import org.apache.kafka.connect.runtime.isolation.ServiceLoaderScanner; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.PrintStream; +import java.io.UncheckedIOException; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; +import java.net.URLConnection; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Properties; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; + +public class ConnectPluginPath { + + private static final String MANIFEST_PREFIX = "META-INF/services/"; + + public static void main(String[] args) { + Exit.exit(mainNoExit(args, System.out, System.err)); + } + + public static int mainNoExit(String[] args, PrintStream out, PrintStream err) { + ArgumentParser parser = parser(); + try { + Namespace namespace = parser.parseArgs(args); + Config config = parseConfig(parser, namespace, out, err); + runCommand(config); + return 0; + } catch (ArgumentParserException e) { + parser.handleError(e); + return 1; + } catch (TerseException e) { + err.println(e.getMessage()); + return 2; + } catch (Throwable e) { + err.println(e.getMessage()); + err.println(Utils.stackTrace(e)); + return 3; + } + } + + private static ArgumentParser parser() { + ArgumentParser parser = ArgumentParsers.newArgumentParser("connect-plugin-path") + .defaultHelp(true) + .description("Manage plugins on the Connect plugin.path"); + + ArgumentParser listCommand = parser.addSubparsers() + .description("List information about plugins contained within the specified plugin locations") + .dest("subcommand") + .addParser("list"); + + ArgumentParser[] subparsers = new ArgumentParser[] { + listCommand, + }; + + for (ArgumentParser subparser : subparsers) { + ArgumentGroup pluginProviders = subparser.addArgumentGroup("plugin providers"); + pluginProviders.addArgument("--plugin-location") + .setDefault(new ArrayList<>()) + .action(Arguments.append()) + .help("A single plugin location (jar file or directory)"); + + pluginProviders.addArgument("--plugin-path") + .setDefault(new ArrayList<>()) + .action(Arguments.append()) + .help("A list of locations containing plugins"); + + pluginProviders.addArgument("--worker-config") + .setDefault(new ArrayList<>()) + .action(Arguments.append()) + .help("A Connect worker configuration file"); + } + + return parser; + } + + private static Config parseConfig(ArgumentParser parser, Namespace namespace, PrintStream out, PrintStream err) throws ArgumentParserException, TerseException { + Set locations = parseLocations(parser, namespace); + switch (namespace.getString("subcommand")) { + case "list": + return new Config(Command.LIST, locations, false, false, out, err); + default: + throw new ArgumentParserException("No subcommand specified", parser); + } + } + + private static Set parseLocations(ArgumentParser parser, Namespace namespace) throws ArgumentParserException, TerseException { + List rawLocations = new ArrayList<>(namespace.getList("plugin_location")); + List rawPluginPaths = new ArrayList<>(namespace.getList("plugin_path")); + List rawWorkerConfigs = new ArrayList<>(namespace.getList("worker_config")); + if (rawLocations.isEmpty() && rawPluginPaths.isEmpty() && rawWorkerConfigs.isEmpty()) { + throw new ArgumentParserException("Must specify at least one --plugin-location, --plugin-path, or --worker-config", parser); + } + Set pluginLocations = new HashSet<>(); + for (String rawWorkerConfig : rawWorkerConfigs) { + Properties properties; + try { + properties = Utils.loadProps(rawWorkerConfig); + } catch (IOException e) { + throw new TerseException("Unable to read worker config at " + rawWorkerConfig); + } + String pluginPath = properties.getProperty(WorkerConfig.PLUGIN_PATH_CONFIG); + if (pluginPath != null) { + rawPluginPaths.add(pluginPath); + } + } + for (String rawPluginPath : rawPluginPaths) { + try { + pluginLocations.addAll(PluginUtils.pluginLocations(rawPluginPath, true)); + } catch (UncheckedIOException e) { + throw new TerseException("Unable to parse plugin path " + rawPluginPath + ": " + e.getMessage()); + } + } + for (String rawLocation : rawLocations) { + pluginLocations.add(Paths.get(rawLocation)); + } + return pluginLocations; + } + + enum Command { + LIST + } + + private static class Config { + private final Command command; + private final Set locations; + private final boolean dryRun; + private final boolean keepNotFound; + private final PrintStream out; + private final PrintStream err; + + private Config(Command command, Set locations, boolean dryRun, boolean keepNotFound, PrintStream out, PrintStream err) { + this.command = command; + this.locations = locations; + this.dryRun = dryRun; + this.keepNotFound = keepNotFound; + this.out = out; + this.err = err; + } + + @Override + public String toString() { + return "Config{" + + "command=" + command + + ", locations=" + locations + + ", dryRun=" + dryRun + + ", keepNotFound=" + keepNotFound + + '}'; + } + } + + public static void runCommand(Config config) throws TerseException { + try { + for (Path pluginLocation : config.locations) { + ClassLoaderFactory factory = new ClassLoaderFactory(); + try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(ConnectPluginPath.class.getClassLoader())) { + Set pluginSources = PluginUtils.pluginSources(Collections.singletonList(pluginLocation), delegatingClassLoader, factory); + Predicate isolated = PluginSource::isolated; + PluginSource isolatedSource = pluginSources.stream() + .filter(isolated) + .findFirst() + .orElseThrow(() -> new IllegalStateException("DelegatingClassLoader should have a PluginSource corresponding to" + pluginLocation)); + PluginSource classpathSource = pluginSources.stream() + .filter(isolated.negate()) + .findFirst() + .orElseThrow(() -> new IllegalStateException("DelegatingClassLoader should have a PluginSource corresponding to the classpath")); + PluginScanResult scanResult = new ReflectionScanner().discoverPlugins(Collections.singleton(isolatedSource)); + PluginScanResult serviceLoadResult = new ServiceLoaderScanner().discoverPlugins(Collections.singleton(isolatedSource)); + PluginScanResult merged = new PluginScanResult(Arrays.asList(scanResult, serviceLoadResult)); + Set isolatedManifests = findManifests(isolatedSource); + Set classpathManifests = findManifests(classpathSource); + classpathManifests.forEach(isolatedManifests::remove); + Map> indexedManifests = indexManifests(isolatedManifests); + Map> aliases = invertAliases(PluginUtils.computeAliases(merged)); + Map> unloadablePlugins = new HashMap<>(indexedManifests); + merged.forEach(pluginDesc -> unloadablePlugins.remove(pluginDesc.className())); + merged.forEach(pluginDesc -> operation(config, isolatedSource, aliases, indexedManifests, pluginDesc.className(), pluginDesc.version(), pluginDesc.type(), true)); + for (Set manifestEntries : unloadablePlugins.values()) { + manifestEntries.forEach(manifestEntry -> operation(config, isolatedSource, aliases, indexedManifests, manifestEntry.className, PluginDesc.UNDEFINED_VERSION, manifestEntry.type, false)); + } + } + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + private static void operation( + // fixed + Config config, + // each location + PluginSource source, + Map> aliases, + Map> manifests, + // each plugin + String pluginName, + String pluginVersion, + PluginType pluginType, + boolean loadable + ) { + operation(config, source, pluginName, pluginVersion, aliases.getOrDefault(pluginName, Collections.emptyList()), pluginType, loadable, manifests.containsKey(pluginName)); + } + + private static void operation( + // fixed + Config config, + // each location + PluginSource source, + // each plugin + String pluginName, + String pluginVersion, + List aliases, + PluginType pluginType, + boolean loadable, + boolean hasManifest + ) { + Path pluginLocation = source.location(); + if (config.command == Command.LIST) { + String firstAlias = aliases.size() > 0 ? aliases.get(0) : "null"; + String secondAlias = aliases.size() > 1 ? aliases.get(1) : "null"; + // there should only be one location, otherwise the plugin source column will be ambiguous. + config.out.printf( + "%s\t%s\t%s\t%s\t%s\t%s\t%b\t%b%n", + pluginLocation, + pluginName, + firstAlias, + secondAlias, + pluginVersion, + pluginType, + loadable, + hasManifest + ); + } + } + + private static class ManifestEntry { + private final URI manifestURI; + private final String className; + private final PluginType type; + + private ManifestEntry(URI manifestURI, String className, PluginType type) { + this.manifestURI = manifestURI; + this.className = className; + this.type = type; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ManifestEntry that = (ManifestEntry) o; + return manifestURI.equals(that.manifestURI) && className.equals(that.className) && type == that.type; + } + + @Override + public int hashCode() { + return Objects.hash(manifestURI, className, type); + } + } + + private static Set findManifests(PluginSource source) throws IOException { + Set manifests = new HashSet<>(); + for (PluginType type : PluginType.values()) { + if (type == PluginType.UNKNOWN) { + continue; + } + try { + Enumeration resources = source.loader().getResources(MANIFEST_PREFIX + type.typeName()); + while (resources.hasMoreElements()) { + URL url = resources.nextElement(); + for (String className : parse(url)) { + manifests.add(new ManifestEntry(url.toURI(), className, type)); + } + } + } catch (URISyntaxException e) { + throw new IOException(e); + } + } + return manifests; + } + + // Based on implementation from ServiceLoader.LazyClassPathLookupIterator + // visible for testing + static Set parse(URL u) { + Set names = new LinkedHashSet<>(); // preserve insertion order + try { + URLConnection uc = u.openConnection(); + uc.setUseCaches(false); + try (InputStream in = uc.getInputStream(); + BufferedReader r + = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))) { + int lc = 1; + while ((lc = parseLine(r, lc, names)) >= 0) { + // pass + } + } + } catch (IOException x) { + throw new RuntimeException("Error accessing ServiceLoader manifest file " + u, x); + } + return names; + } + + private static int parseLine(BufferedReader r, int lc, Set names) throws IOException { + String ln = r.readLine(); + if (ln == null) { + return -1; + } + int ci = ln.indexOf('#'); + if (ci >= 0) ln = ln.substring(0, ci); + ln = ln.trim(); + int n = ln.length(); + if (n != 0) { + if ((ln.indexOf(' ') >= 0) || (ln.indexOf('\t') >= 0)) + throw new IOException("Illegal configuration-file syntax"); + int cp = ln.codePointAt(0); + if (!Character.isJavaIdentifierStart(cp)) + throw new IOException("Illegal provider-class name: " + ln); + int start = Character.charCount(cp); + for (int i = start; i < n; i += Character.charCount(cp)) { + cp = ln.codePointAt(i); + if (!Character.isJavaIdentifierPart(cp) && (cp != '.')) + throw new IOException("Illegal provider-class name: " + ln); + } + names.add(ln); + } + return lc + 1; + } + + private static Map> indexManifests(Set manifests) { + Map> classToManifests = new HashMap<>(); + for (ManifestEntry manifestEntry : manifests) { + Set uris = classToManifests.computeIfAbsent(manifestEntry.className, ignored -> new HashSet<>()); + uris.add(manifestEntry); + } + return classToManifests; + } + + private static Map> invertAliases(Map aliases) { + return aliases.entrySet() + .stream() + .collect(Collectors.toMap(Map.Entry::getValue, e -> Collections.singletonList(e.getKey()), (a, b) -> { + if (a instanceof ArrayList) { + a.addAll(b); + return a; + } else { + List result = new ArrayList<>(); + result.addAll(a); + result.addAll(b); + return result; + } + })); + } +} diff --git a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java new file mode 100644 index 0000000000000..29d7567a0d486 --- /dev/null +++ b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java @@ -0,0 +1,371 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.kafka.tools; + +import org.apache.kafka.connect.runtime.isolation.ClassLoaderFactory; +import org.apache.kafka.connect.runtime.isolation.DelegatingClassLoader; +import org.apache.kafka.connect.runtime.isolation.PluginScanResult; +import org.apache.kafka.connect.runtime.isolation.PluginSource; +import org.apache.kafka.connect.runtime.isolation.PluginUtils; +import org.apache.kafka.connect.runtime.isolation.ReflectionScanner; +import org.apache.kafka.connect.runtime.isolation.ServiceLoaderScanner; +import org.apache.kafka.connect.runtime.isolation.TestPlugins; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.io.PrintStream; +import java.io.UncheckedIOException; +import java.io.UnsupportedEncodingException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Objects; +import java.util.Properties; +import java.util.Set; +import java.util.jar.JarFile; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; + +public class ConnectPluginPathTest { + + @TempDir + public Path workspace; + + private enum PluginLocationType { + CLASS_HIERARCHY, + SINGLE_JAR, + MULTI_JAR + } + + private static class PluginLocation { + private final Path path; + + private PluginLocation(Path path) { + this.path = path; + } + + @Override + public String toString() { + return path.toString(); + } + } + + @BeforeAll + public static void setUp() { + // Work around a circular-dependency in TestPlugins. + TestPlugins.pluginPath(); + } + + /** + * Populate a writable disk path to be usable as a single plugin location. + * The returned path will be usable as a single path. + * @param path A non-existent path immediately within a writable directory, suggesting a location for this plugin. + * @param type The format to which the on-disk plugin should conform + * @param plugin The plugin which should be written to the specified path + * @return The final usable path name to this location, in case it is different from the suggested input path. + */ + private static PluginLocation setupLocation(Path path, PluginLocationType type, TestPlugins.TestPlugin plugin) { + try { + Path jarPath = TestPlugins.pluginPath(plugin).get(0); + switch (type) { + case CLASS_HIERARCHY: { + try (JarFile jarFile = new JarFile(jarPath.toFile())) { + jarFile.stream().forEach(jarEntry -> { + Path entryPath = path.resolve(jarEntry.getName()); + try { + entryPath.getParent().toFile().mkdirs(); + Files.copy(jarFile.getInputStream(jarEntry), entryPath, StandardCopyOption.REPLACE_EXISTING); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); + } + return new PluginLocation(path); + } + case SINGLE_JAR: { + Path outputJar = path.resolveSibling(path.getFileName() + ".jar"); + outputJar.getParent().toFile().mkdirs(); + Files.copy(jarPath, outputJar, StandardCopyOption.REPLACE_EXISTING); + return new PluginLocation(outputJar); + } + case MULTI_JAR: { + Path outputJar = path.resolve(jarPath.getFileName()); + outputJar.getParent().toFile().mkdirs(); + Files.copy(jarPath, outputJar, StandardCopyOption.REPLACE_EXISTING); + return new PluginLocation(path); + } + default: + throw new IllegalArgumentException("Unknown PluginLocationType"); + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + private static class PluginPathElement { + private final Path root; + private final List locations; + + private PluginPathElement(Path root, List locations) { + this.root = root; + this.locations = locations; + } + + @Override + public String toString() { + return root.toString(); + } + } + + /** + * Populate a writable disk path to be usable as single {@code plugin.path} element providing the specified plugins + * @param path A directory that should contain the populated plugins, will be created if it does not exist. + * @param type The format to which the on-disk plugins should conform + * @param plugins The plugins which should be written to the specified path + * @return The specific inner locations of the plugins that were written. + */ + private PluginPathElement setupPluginPathElement(Path path, PluginLocationType type, TestPlugins.TestPlugin... plugins) { + List locations = new ArrayList<>(); + for (int i = 0; i < plugins.length; i++) { + TestPlugins.TestPlugin plugin = plugins[i]; + locations.add(setupLocation(path.resolve("plugin-" + i), type, plugin)); + } + return new PluginPathElement(path, locations); + } + + private static class WorkerConfig { + private final Path configFile; + private final List pluginPathElements; + + private WorkerConfig(Path configFile, List pluginPathElements) { + this.configFile = configFile; + this.pluginPathElements = pluginPathElements; + } + + @Override + public String toString() { + return configFile.toString(); + } + } + + /** + * Populate a writable disk path + * @param path + * @param pluginPathElements + * @return + */ + private WorkerConfig setupWorkerConfig(Path path, PluginPathElement... pluginPathElements) { + path.getParent().toFile().mkdirs(); + Properties properties = new Properties(); + String pluginPath = Arrays.stream(pluginPathElements) + .map(Object::toString) + .collect(Collectors.joining(", ")); + properties.setProperty("plugin.path", pluginPath); + try (OutputStream outputStream = Files.newOutputStream(path)) { + properties.store(outputStream, "dummy worker properties file"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return new WorkerConfig(path, Arrays.asList(pluginPathElements)); + } + + private static class CommandResult { + public CommandResult(int returnCode, String out, String err, PluginScanResult reflective, PluginScanResult serviceLoading) { + this.returnCode = returnCode; + this.out = out; + this.err = err; + this.reflective = reflective; + this.serviceLoading = serviceLoading; + } + + int returnCode; + String out; + String err; + PluginScanResult reflective; + PluginScanResult serviceLoading; + } + + private CommandResult runCommand(Object... args) { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + ByteArrayOutputStream err = new ByteArrayOutputStream(); + try { + int returnCode = ConnectPluginPath.mainNoExit( + Arrays.stream(args) + .map(Object::toString) + .collect(Collectors.toList()) + .toArray(new String[]{}), + new PrintStream(out, true, "utf-8"), + new PrintStream(err, true, "utf-8")); + List pluginLocations = getPluginLocations(args); + ClassLoader parent = ConnectPluginPath.class.getClassLoader(); + ClassLoaderFactory factory = new ClassLoaderFactory(); + try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { + Set sources = PluginUtils.pluginSources(pluginLocations, delegatingClassLoader, factory) + .stream() + .filter(PluginSource::isolated) + .collect(Collectors.toSet()); + return new CommandResult( + returnCode, + new String(out.toByteArray(), StandardCharsets.UTF_8), + new String(err.toByteArray(), StandardCharsets.UTF_8), + new ReflectionScanner().discoverPlugins(sources), + new ServiceLoaderScanner().discoverPlugins(sources) + ); + } catch (IOException e) { + throw new RuntimeException(e); + } + } catch (UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + } + + private static List getPluginLocations(Object[] args) { + return Arrays.stream(args) + .flatMap(obj -> { + if (obj instanceof WorkerConfig) { + return ((WorkerConfig) obj).pluginPathElements.stream(); + } else { + return Stream.of(obj); + } + }) + .flatMap(obj -> { + if (obj instanceof PluginPathElement) { + return ((PluginPathElement) obj).locations.stream(); + } else { + return Stream.of(obj); + } + }) + .map(obj -> { + if (obj instanceof PluginLocation) { + return ((PluginLocation) obj).path; + } else { + return null; + } + }) + + .filter(Objects::nonNull) + .collect(Collectors.toList()); + } + + @Test + public void testNoArguments() { + CommandResult res = runCommand(); + assertNotEquals(0, res.returnCode); + } + + @Test + public void testListNoArguments() { + CommandResult res = runCommand( + "list" + ); + assertNotEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListOneLocation(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-location", + setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.SAMPLING_CONVERTER) + ); + assertEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListMultipleLocations(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-location", + setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.SAMPLING_CONVERTER), + "--plugin-location", + setupLocation(workspace.resolve("location-b"), type, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) + ); + assertEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListOnePluginPath(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-path", + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.SAMPLING_CONVERTER, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) + ); + assertEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListMultiplePluginPaths(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-path", + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.SAMPLING_CONVERTER, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE), + "--plugin-path", + setupPluginPathElement(workspace.resolve("path-b"), type, + TestPlugins.TestPlugin.SAMPLING_HEADER_CONVERTER, TestPlugins.TestPlugin.ALIASED_STATIC_FIELD) + ); + assertEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListOneWorkerConfig(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--worker-config", + setupWorkerConfig(workspace.resolve("worker.properties"), + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.SAMPLING_CONVERTER)) + ); + assertEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListMultipleWorkerConfigs(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--worker-config", + setupWorkerConfig(workspace.resolve("worker-a.properties"), + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.SAMPLING_CONVERTER)), + "--worker-config", + setupWorkerConfig(workspace.resolve("worker-b.properties"), + setupPluginPathElement(workspace.resolve("path-b"), type, + TestPlugins.TestPlugin.SERVICE_LOADER)) + ); + assertEquals(0, res.returnCode); + } +} From 417c264a77eb792aae48bced22f7545d3979f676 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 20 Jul 2023 13:48:43 -0700 Subject: [PATCH 02/28] fixup: remove unused variables Signed-off-by: Greg Harris --- .../java/org/apache/kafka/tools/ConnectPluginPath.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 6848a1dd55208..11f76b0dfd907 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -130,7 +130,7 @@ private static Config parseConfig(ArgumentParser parser, Namespace namespace, Pr Set locations = parseLocations(parser, namespace); switch (namespace.getString("subcommand")) { case "list": - return new Config(Command.LIST, locations, false, false, out, err); + return new Config(Command.LIST, locations, out, err); default: throw new ArgumentParserException("No subcommand specified", parser); } @@ -176,16 +176,12 @@ enum Command { private static class Config { private final Command command; private final Set locations; - private final boolean dryRun; - private final boolean keepNotFound; private final PrintStream out; private final PrintStream err; - private Config(Command command, Set locations, boolean dryRun, boolean keepNotFound, PrintStream out, PrintStream err) { + private Config(Command command, Set locations, PrintStream out, PrintStream err) { this.command = command; this.locations = locations; - this.dryRun = dryRun; - this.keepNotFound = keepNotFound; this.out = out; this.err = err; } @@ -195,8 +191,6 @@ public String toString() { return "Config{" + "command=" + command + ", locations=" + locations + - ", dryRun=" + dryRun + - ", keepNotFound=" + keepNotFound + '}'; } } From 72cb970407318a0a3cd639aa23b790828c72c910 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 20 Jul 2023 14:06:00 -0700 Subject: [PATCH 03/28] fixup: remove NoManifestConverter Signed-off-by: Greg Harris --- .../test/plugins/NoManifestConverter.java | 44 ------------------- 1 file changed, 44 deletions(-) delete mode 100644 connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java diff --git a/connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java b/connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java deleted file mode 100644 index e0f0cbbf5ff70..0000000000000 --- a/connect/runtime/src/test/resources/test-plugins/no-service-loader-manifests/test/plugins/NoManifestConverter.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package test.plugins; - -import java.util.Map; -import org.apache.kafka.connect.data.Schema; -import org.apache.kafka.connect.data.SchemaAndValue; -import org.apache.kafka.connect.storage.Converter; - -/** - * Fake plugin class for testing classloading isolation. - * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. - */ -public class NoManifestConverter implements Converter { - - @Override - public void configure(final Map configs, final boolean isKey) { - } - - @Override - public byte[] fromConnectData(final String topic, final Schema schema, final Object value) { - return new byte[0]; - } - - @Override - public SchemaAndValue toConnectData(final String topic, final byte[] value) { - return null; - } -} From 3f28fe633c30f37238f781c74649e5d854d84a7f Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 24 Jul 2023 11:53:38 -0700 Subject: [PATCH 04/28] fixup: review comments Signed-off-by: Greg Harris --- bin/windows/connect-plugin-path.bat | 2 +- .../apache/kafka/tools/ConnectPluginPath.java | 71 ++++++++++--------- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/bin/windows/connect-plugin-path.bat b/bin/windows/connect-plugin-path.bat index aeb324450cdfe..35f16707693fc 100644 --- a/bin/windows/connect-plugin-path.bat +++ b/bin/windows/connect-plugin-path.bat @@ -14,4 +14,4 @@ rem WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. rem See the License for the specific language governing permissions and rem limitations under the License. -%~dp0kafka-run-class.bat org.apache.kafka.tools.ConnectPluginPath %* \ No newline at end of file +"%~dp0kafka-run-class.bat" org.apache.kafka.tools.ConnectPluginPath %* \ No newline at end of file diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 11f76b0dfd907..0114e16b14154 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -62,6 +62,7 @@ import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; +import java.util.stream.Stream; public class ConnectPluginPath { @@ -75,7 +76,7 @@ public static int mainNoExit(String[] args, PrintStream out, PrintStream err) { ArgumentParser parser = parser(); try { Namespace namespace = parser.parseArgs(args); - Config config = parseConfig(parser, namespace, out, err); + Config config = parseConfig(parser, namespace, out); runCommand(config); return 0; } catch (ArgumentParserException e) { @@ -115,7 +116,7 @@ private static ArgumentParser parser() { pluginProviders.addArgument("--plugin-path") .setDefault(new ArrayList<>()) .action(Arguments.append()) - .help("A list of locations containing plugins"); + .help("A comma-delimited list of locations containing plugins"); pluginProviders.addArgument("--worker-config") .setDefault(new ArrayList<>()) @@ -126,13 +127,17 @@ private static ArgumentParser parser() { return parser; } - private static Config parseConfig(ArgumentParser parser, Namespace namespace, PrintStream out, PrintStream err) throws ArgumentParserException, TerseException { + private static Config parseConfig(ArgumentParser parser, Namespace namespace, PrintStream out) throws ArgumentParserException, TerseException { Set locations = parseLocations(parser, namespace); - switch (namespace.getString("subcommand")) { + String subcommand = namespace.getString("subcommand"); + if (subcommand == null) { + throw new ArgumentParserException("No subcommand specified", parser); + } + switch (subcommand) { case "list": - return new Config(Command.LIST, locations, out, err); + return new Config(Command.LIST, locations, out); default: - throw new ArgumentParserException("No subcommand specified", parser); + throw new ArgumentParserException("Unrecognized subcommand: '" + subcommand + "'", parser); } } @@ -164,7 +169,11 @@ private static Set parseLocations(ArgumentParser parser, Namespace namespa } } for (String rawLocation : rawLocations) { - pluginLocations.add(Paths.get(rawLocation)); + Path pluginLocation = Paths.get(rawLocation); + if (!pluginLocation.toFile().exists()) { + throw new TerseException("Specified location " + pluginLocation + " does not exist"); + } + pluginLocations.add(pluginLocation); } return pluginLocations; } @@ -177,13 +186,11 @@ private static class Config { private final Command command; private final Set locations; private final PrintStream out; - private final PrintStream err; - private Config(Command command, Set locations, PrintStream out, PrintStream err) { + private Config(Command command, Set locations, PrintStream out) { this.command = command; this.locations = locations; this.out = out; - this.err = err; } @Override @@ -216,13 +223,13 @@ public static void runCommand(Config config) throws TerseException { Set isolatedManifests = findManifests(isolatedSource); Set classpathManifests = findManifests(classpathSource); classpathManifests.forEach(isolatedManifests::remove); - Map> indexedManifests = indexManifests(isolatedManifests); - Map> aliases = invertAliases(PluginUtils.computeAliases(merged)); + Map> indexedManifests = manifestsByClassName(isolatedManifests); + Map> aliases = aliasesByClassName(PluginUtils.computeAliases(merged)); + merged.forEach(pluginDesc -> handlePlugin(config, isolatedSource, aliases, indexedManifests, pluginDesc.className(), pluginDesc.version(), pluginDesc.type(), true)); Map> unloadablePlugins = new HashMap<>(indexedManifests); merged.forEach(pluginDesc -> unloadablePlugins.remove(pluginDesc.className())); - merged.forEach(pluginDesc -> operation(config, isolatedSource, aliases, indexedManifests, pluginDesc.className(), pluginDesc.version(), pluginDesc.type(), true)); for (Set manifestEntries : unloadablePlugins.values()) { - manifestEntries.forEach(manifestEntry -> operation(config, isolatedSource, aliases, indexedManifests, manifestEntry.className, PluginDesc.UNDEFINED_VERSION, manifestEntry.type, false)); + manifestEntries.forEach(manifestEntry -> handlePlugin(config, isolatedSource, aliases, indexedManifests, manifestEntry.className, PluginDesc.UNDEFINED_VERSION, manifestEntry.type, false)); } } } @@ -231,7 +238,7 @@ public static void runCommand(Config config) throws TerseException { } } - private static void operation( + private static void handlePlugin( // fixed Config config, // each location @@ -244,10 +251,10 @@ private static void operation( PluginType pluginType, boolean loadable ) { - operation(config, source, pluginName, pluginVersion, aliases.getOrDefault(pluginName, Collections.emptyList()), pluginType, loadable, manifests.containsKey(pluginName)); + handlePlugin(config, source, pluginName, pluginVersion, aliases.getOrDefault(pluginName, Collections.emptyList()), pluginType, loadable, manifests.containsKey(pluginName)); } - private static void operation( + private static void handlePlugin( // fixed Config config, // each location @@ -264,9 +271,7 @@ private static void operation( if (config.command == Command.LIST) { String firstAlias = aliases.size() > 0 ? aliases.get(0) : "null"; String secondAlias = aliases.size() > 1 ? aliases.get(1) : "null"; - // there should only be one location, otherwise the plugin source column will be ambiguous. - config.out.printf( - "%s\t%s\t%s\t%s\t%s\t%s\t%b\t%b%n", + String pluginInfo = Stream.of( pluginLocation, pluginName, firstAlias, @@ -275,7 +280,9 @@ private static void operation( pluginType, loadable, hasManifest - ); + ).map(Objects::toString).collect(Collectors.joining("\t")); + // there should only be one location, otherwise the plugin source column will be ambiguous. + config.out.println(pluginInfo); } } @@ -372,7 +379,7 @@ private static int parseLine(BufferedReader r, int lc, Set names) throws return lc + 1; } - private static Map> indexManifests(Set manifests) { + private static Map> manifestsByClassName(Set manifests) { Map> classToManifests = new HashMap<>(); for (ManifestEntry manifestEntry : manifests) { Set uris = classToManifests.computeIfAbsent(manifestEntry.className, ignored -> new HashSet<>()); @@ -381,19 +388,15 @@ private static Map> indexManifests(Set return classToManifests; } - private static Map> invertAliases(Map aliases) { + private static Map> aliasesByClassName(Map aliases) { return aliases.entrySet() .stream() - .collect(Collectors.toMap(Map.Entry::getValue, e -> Collections.singletonList(e.getKey()), (a, b) -> { - if (a instanceof ArrayList) { - a.addAll(b); - return a; - } else { - List result = new ArrayList<>(); - result.addAll(a); - result.addAll(b); - return result; - } - })); + .collect(Collectors.toMap( + Map.Entry::getValue, + e -> new ArrayList<>(Collections.singletonList(e.getKey())), + (a, b) -> { + a.addAll(b); + return a; + })); } } From b10415316ce5b15c04680eb469511d17e1a72af0 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 24 Jul 2023 15:48:32 -0700 Subject: [PATCH 05/28] fixup: restructure iteration to compute aliases across all scan results Signed-off-by: Greg Harris --- .../runtime/isolation/PluginUtils.java | 26 +++-- .../apache/kafka/tools/ConnectPluginPath.java | 108 ++++++++++-------- .../kafka/tools/ConnectPluginPathTest.java | 17 ++- 3 files changed, 92 insertions(+), 59 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index 2aa1cf8ef7ec4..37d2ab0062b3e 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -205,7 +205,12 @@ public static List pluginLocations(String pluginPath, boolean failFast) { List pluginLocations = new ArrayList<>(); for (String path : pluginPathElements) { try { - Path pluginPathElement = Paths.get(path).toAbsolutePath(); + Path specifiedPath = Paths.get(path); + Path pluginPathElement = specifiedPath.toAbsolutePath(); + if (!specifiedPath.isAbsolute()) { + log.warn("Plugin path element '{}' is relative, evaluating to {}.", + specifiedPath, pluginPathElement); + } if (!Files.exists(pluginPathElement)) { throw new FileNotFoundException(pluginPathElement.toString()); } @@ -336,9 +341,14 @@ public static List pluginUrls(Path topPath) throws IOException { } public static Set pluginSources(List pluginLocations, ClassLoader classLoader, PluginClassLoaderFactory factory) { + Set pluginSources = new HashSet<>(isolatedPluginSources(pluginLocations, classLoader, factory)); + pluginSources.add(classpathPluginSource(classLoader.getParent())); + return pluginSources; + } + + public static Set isolatedPluginSources(List pluginLocations, ClassLoader parent, PluginClassLoaderFactory factory) { Set pluginSources = new HashSet<>(); for (Path pluginLocation : pluginLocations) { - try { List pluginUrls = new ArrayList<>(); for (Path path : pluginUrls(pluginLocation)) { @@ -348,7 +358,7 @@ public static Set pluginSources(List pluginLocations, ClassL PluginClassLoader loader = factory.newPluginClassLoader( pluginLocation.toUri().toURL(), urls, - classLoader + parent ); pluginSources.add(new PluginSource(pluginLocation, loader, urls)); } catch (InvalidPathException | MalformedURLException e) { @@ -357,13 +367,15 @@ public static Set pluginSources(List pluginLocations, ClassL log.error("Could not get listing for plugin path: {}. Ignoring.", pluginLocation, e); } } - List parentUrls = new ArrayList<>(); - parentUrls.addAll(ClasspathHelper.forJavaClassPath()); - parentUrls.addAll(ClasspathHelper.forClassLoader(classLoader.getParent())); - pluginSources.add(new PluginSource(PluginSource.CLASSPATH, classLoader.getParent(), parentUrls.toArray(new URL[0]))); return pluginSources; } + public static PluginSource classpathPluginSource(ClassLoader classLoader) { + List parentUrls = new ArrayList<>(); + parentUrls.addAll(ClasspathHelper.forJavaClassPath()); + parentUrls.addAll(ClasspathHelper.forClassLoader(classLoader)); + return new PluginSource(PluginSource.CLASSPATH, classLoader, parentUrls.toArray(new URL[0])); + } /** * Return the simple class name of a plugin as {@code String}. diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 0114e16b14154..bbc248c50a8d1 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -50,6 +50,7 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; @@ -60,7 +61,7 @@ import java.util.Objects; import java.util.Properties; import java.util.Set; -import java.util.function.Predicate; +import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -204,40 +205,48 @@ public String toString() { public static void runCommand(Config config) throws TerseException { try { - for (Path pluginLocation : config.locations) { - ClassLoaderFactory factory = new ClassLoaderFactory(); - try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(ConnectPluginPath.class.getClassLoader())) { - Set pluginSources = PluginUtils.pluginSources(Collections.singletonList(pluginLocation), delegatingClassLoader, factory); - Predicate isolated = PluginSource::isolated; - PluginSource isolatedSource = pluginSources.stream() - .filter(isolated) - .findFirst() - .orElseThrow(() -> new IllegalStateException("DelegatingClassLoader should have a PluginSource corresponding to" + pluginLocation)); - PluginSource classpathSource = pluginSources.stream() - .filter(isolated.negate()) - .findFirst() - .orElseThrow(() -> new IllegalStateException("DelegatingClassLoader should have a PluginSource corresponding to the classpath")); - PluginScanResult scanResult = new ReflectionScanner().discoverPlugins(Collections.singleton(isolatedSource)); - PluginScanResult serviceLoadResult = new ServiceLoaderScanner().discoverPlugins(Collections.singleton(isolatedSource)); - PluginScanResult merged = new PluginScanResult(Arrays.asList(scanResult, serviceLoadResult)); - Set isolatedManifests = findManifests(isolatedSource); - Set classpathManifests = findManifests(classpathSource); - classpathManifests.forEach(isolatedManifests::remove); - Map> indexedManifests = manifestsByClassName(isolatedManifests); - Map> aliases = aliasesByClassName(PluginUtils.computeAliases(merged)); - merged.forEach(pluginDesc -> handlePlugin(config, isolatedSource, aliases, indexedManifests, pluginDesc.className(), pluginDesc.version(), pluginDesc.type(), true)); - Map> unloadablePlugins = new HashMap<>(indexedManifests); - merged.forEach(pluginDesc -> unloadablePlugins.remove(pluginDesc.className())); - for (Set manifestEntries : unloadablePlugins.values()) { - manifestEntries.forEach(manifestEntry -> handlePlugin(config, isolatedSource, aliases, indexedManifests, manifestEntry.className, PluginDesc.UNDEFINED_VERSION, manifestEntry.type, false)); - } + // Process the contents of the classpath to exclude it from later results. + ClassLoader parent = ConnectPluginPath.class.getClassLoader(); + PluginSource classpathSource = PluginUtils.classpathPluginSource(parent); + Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); + PluginScanResult classpathPlugins = discoverPlugins(classpathSource); + + ClassLoaderFactory factory = new ClassLoaderFactory(); + try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { + // Process the contents of the isolated locations to find plugins + Set isolatedSources = PluginUtils.isolatedPluginSources(new ArrayList<>(config.locations), delegatingClassLoader, factory); + // Exclude manifest entries which are visible from the classpath + Map>> isolatedManifests = isolatedSources.stream() + .collect(Collectors.toMap(Function.identity(), source -> findManifests(source, classpathManifests))); + Map isolatedPlugins = isolatedSources.stream() + .collect(Collectors.toMap(Function.identity(), ConnectPluginPath::discoverPlugins)); + // Calculate aliases including collisions with classpath plugins + Map> aliases = computeAliases(classpathPlugins, isolatedPlugins.values()); + beginCommand(config); + for (PluginSource isolatedSource : isolatedSources) { + handlePluginSource(config, isolatedSource, isolatedManifests.get(isolatedSource), isolatedPlugins.get(isolatedSource), aliases); } + endCommand(config); } } catch (IOException e) { throw new UncheckedIOException(e); } } + private static void beginCommand(Config config) { + + } + + private static void handlePluginSource(Config config, PluginSource isolatedSource, Map> indexedManifests, PluginScanResult merged, Map> aliases) { + merged.forEach(pluginDesc -> handlePlugin(config, isolatedSource, aliases, indexedManifests, pluginDesc.className(), pluginDesc.version(), pluginDesc.type(), true)); + Map> unloadablePlugins = new HashMap<>(indexedManifests); + merged.forEach(pluginDesc -> unloadablePlugins.remove(pluginDesc.className())); + for (Set manifestEntries : unloadablePlugins.values()) { + manifestEntries.forEach(manifestEntry -> handlePlugin(config, isolatedSource, aliases, indexedManifests, manifestEntry.className, PluginDesc.UNDEFINED_VERSION, manifestEntry.type, false)); + } + } + + private static void handlePlugin( // fixed Config config, @@ -272,20 +281,29 @@ private static void handlePlugin( String firstAlias = aliases.size() > 0 ? aliases.get(0) : "null"; String secondAlias = aliases.size() > 1 ? aliases.get(1) : "null"; String pluginInfo = Stream.of( - pluginLocation, pluginName, firstAlias, secondAlias, pluginVersion, pluginType, loadable, - hasManifest + hasManifest, + pluginLocation // last because it is least important and most repetitive ).map(Objects::toString).collect(Collectors.joining("\t")); - // there should only be one location, otherwise the plugin source column will be ambiguous. config.out.println(pluginInfo); } } + private static void endCommand(Config config) { + + } + + private static PluginScanResult discoverPlugins(PluginSource source) { + PluginScanResult serviceLoadResult = new ServiceLoaderScanner().discoverPlugins(Collections.singleton(source)); + PluginScanResult reflectiveResult = new ReflectionScanner().discoverPlugins(Collections.singleton(source)); + return new PluginScanResult(Arrays.asList(serviceLoadResult, reflectiveResult)); + } + private static class ManifestEntry { private final URI manifestURI; private final String className; @@ -311,8 +329,8 @@ public int hashCode() { } } - private static Set findManifests(PluginSource source) throws IOException { - Set manifests = new HashSet<>(); + private static Map> findManifests(PluginSource source, Map> exclude) { + Map> manifests = new HashMap<>(); for (PluginType type : PluginType.values()) { if (type == PluginType.UNKNOWN) { continue; @@ -322,11 +340,16 @@ private static Set findManifests(PluginSource source) throws IOEx while (resources.hasMoreElements()) { URL url = resources.nextElement(); for (String className : parse(url)) { - manifests.add(new ManifestEntry(url.toURI(), className, type)); + ManifestEntry e = new ManifestEntry(url.toURI(), className, type); + if (!exclude.containsKey(className) || !exclude.get(className).contains(e)) { + manifests.computeIfAbsent(className, ignored -> new HashSet<>()).add(e); + } } } } catch (URISyntaxException e) { - throw new IOException(e); + throw new RuntimeException(e); + } catch (IOException e) { + throw new UncheckedIOException(e); } } return manifests; @@ -379,17 +402,10 @@ private static int parseLine(BufferedReader r, int lc, Set names) throws return lc + 1; } - private static Map> manifestsByClassName(Set manifests) { - Map> classToManifests = new HashMap<>(); - for (ManifestEntry manifestEntry : manifests) { - Set uris = classToManifests.computeIfAbsent(manifestEntry.className, ignored -> new HashSet<>()); - uris.add(manifestEntry); - } - return classToManifests; - } - - private static Map> aliasesByClassName(Map aliases) { - return aliases.entrySet() + private static Map> computeAliases(PluginScanResult classpathPlugins, Collection isolatedPlugins) { + List allResults = new ArrayList<>(isolatedPlugins); + allResults.add(classpathPlugins); + return PluginUtils.computeAliases(new PluginScanResult(allResults)).entrySet() .stream() .collect(Collectors.toMap( Map.Entry::getValue, diff --git a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java index 29d7567a0d486..3b16cb2e98dc3 100644 --- a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java +++ b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java @@ -29,6 +29,8 @@ import org.junit.jupiter.api.io.TempDir; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -55,6 +57,8 @@ public class ConnectPluginPathTest { + private static final Logger log = LoggerFactory.getLogger(ConnectPluginPathTest.class); + @TempDir public Path workspace; @@ -227,14 +231,15 @@ private CommandResult runCommand(Object... args) { ClassLoader parent = ConnectPluginPath.class.getClassLoader(); ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { - Set sources = PluginUtils.pluginSources(pluginLocations, delegatingClassLoader, factory) - .stream() - .filter(PluginSource::isolated) - .collect(Collectors.toSet()); + Set sources = PluginUtils.isolatedPluginSources(pluginLocations, delegatingClassLoader, factory); + String stdout = new String(out.toByteArray(), StandardCharsets.UTF_8); + String stderr = new String(err.toByteArray(), StandardCharsets.UTF_8); + log.info("STDOUT:\n{}", stdout); + log.info("STDERR:\n{}", stderr); return new CommandResult( returnCode, - new String(out.toByteArray(), StandardCharsets.UTF_8), - new String(err.toByteArray(), StandardCharsets.UTF_8), + stdout, + stderr, new ReflectionScanner().discoverPlugins(sources), new ServiceLoaderScanner().discoverPlugins(sources) ); From 3ab85d39885ace92709d34c403d60ca0a604683c Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Tue, 25 Jul 2023 17:59:22 -0700 Subject: [PATCH 06/28] fixup: redo iteration with new Row construct, print summary table at the end of list Signed-off-by: Greg Harris --- .../apache/kafka/tools/ConnectPluginPath.java | 215 +++++++++++++----- 1 file changed, 156 insertions(+), 59 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index bbc248c50a8d1..94ed36be2704f 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -61,6 +61,7 @@ import java.util.Objects; import java.util.Properties; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -68,6 +69,8 @@ public class ConnectPluginPath { private static final String MANIFEST_PREFIX = "META-INF/services/"; + private static final int LIST_TABLE_COLUMN_COUNT = 8; + private static final int LIST_SUMMARY_COLUMN_COUNT = 4; public static void main(String[] args) { Exit.exit(mainNoExit(args, System.out, System.err)); @@ -205,102 +208,196 @@ public String toString() { public static void runCommand(Config config) throws TerseException { try { - // Process the contents of the classpath to exclude it from later results. ClassLoader parent = ConnectPluginPath.class.getClassLoader(); + ServiceLoaderScanner serviceLoaderScanner = new ServiceLoaderScanner(); + ReflectionScanner reflectionScanner = new ReflectionScanner(); + // Process the contents of the classpath to exclude it from later results. PluginSource classpathSource = PluginUtils.classpathPluginSource(parent); Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); - PluginScanResult classpathPlugins = discoverPlugins(classpathSource); + PluginScanResult classpathPlugins = discoverPlugins(classpathSource, reflectionScanner, serviceLoaderScanner); ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { - // Process the contents of the isolated locations to find plugins Set isolatedSources = PluginUtils.isolatedPluginSources(new ArrayList<>(config.locations), delegatingClassLoader, factory); - // Exclude manifest entries which are visible from the classpath Map>> isolatedManifests = isolatedSources.stream() - .collect(Collectors.toMap(Function.identity(), source -> findManifests(source, classpathManifests))); + .collect(Collectors.toMap(Function.identity(), + // Exclude manifest entries which are visible from the classpath + source -> findManifests(source, classpathManifests))); Map isolatedPlugins = isolatedSources.stream() - .collect(Collectors.toMap(Function.identity(), ConnectPluginPath::discoverPlugins)); + .collect(Collectors.toMap(Function.identity(), + source -> discoverPlugins(source, reflectionScanner, serviceLoaderScanner))); // Calculate aliases including collisions with classpath plugins Map> aliases = computeAliases(classpathPlugins, isolatedPlugins.values()); - beginCommand(config); - for (PluginSource isolatedSource : isolatedSources) { - handlePluginSource(config, isolatedSource, isolatedManifests.get(isolatedSource), isolatedPlugins.get(isolatedSource), aliases); - } - endCommand(config); + // Merge the plugins and manifest results to get the individual units of work + Map> isolatedRows = isolatedSources.stream() + .collect(Collectors.toMap(Function.identity(), + source -> enumerateRows(source, isolatedManifests.get(source), aliases, isolatedPlugins.get(source)))); + runCommand(config, isolatedManifests, isolatedPlugins, isolatedRows); } } catch (IOException e) { throw new UncheckedIOException(e); } } - private static void beginCommand(Config config) { + /** + * The unit of work for a command. + *

This is unique to the (source, class, type) tuple, and contains additional pre-computed information + * that pertains to this specific plugin. + */ + private static class Row { + private final PluginSource source; + private final String className; + private final PluginType type; + private final String version; + private final List aliases; + private final boolean loadable; + private final boolean hasManifest; - } + public Row(PluginSource source, String className, PluginType type, String version, List aliases, boolean loadable, boolean hasManifest) { + this.source = source; + this.className = className; + this.version = version; + this.type = type; + this.aliases = aliases; + this.loadable = loadable; + this.hasManifest = hasManifest; + } - private static void handlePluginSource(Config config, PluginSource isolatedSource, Map> indexedManifests, PluginScanResult merged, Map> aliases) { - merged.forEach(pluginDesc -> handlePlugin(config, isolatedSource, aliases, indexedManifests, pluginDesc.className(), pluginDesc.version(), pluginDesc.type(), true)); - Map> unloadablePlugins = new HashMap<>(indexedManifests); - merged.forEach(pluginDesc -> unloadablePlugins.remove(pluginDesc.className())); - for (Set manifestEntries : unloadablePlugins.values()) { - manifestEntries.forEach(manifestEntry -> handlePlugin(config, isolatedSource, aliases, indexedManifests, manifestEntry.className, PluginDesc.UNDEFINED_VERSION, manifestEntry.type, false)); + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Row row = (Row) o; + return source.equals(row.source) && className.equals(row.className) && type == row.type; + } + + @Override + public int hashCode() { + return Objects.hash(source, className, type); } } + private static Set enumerateRows(PluginSource source, Map> manifests, Map> aliases, PluginScanResult scanResult) { + Set rows = new HashSet<>(); + // Perform a deep copy of the manifests because we're going to be mutating our copy. + Map> unloadablePlugins = manifests.entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, e -> new HashSet<>(e.getValue()))); + scanResult.forEach(pluginDesc -> { + // Emit a loadable row for this scan result, since it was found during plugin discovery + rows.add(newRow(source, pluginDesc.className(), pluginDesc.type(), pluginDesc.version(), true, manifests, aliases)); + // Remove the ManifestEntry if it has the same className and type as one of the loadable plugins. + unloadablePlugins.get(pluginDesc.className()).removeIf(entry -> entry.type == pluginDesc.type()); + }); + unloadablePlugins.values().forEach(entries -> entries.forEach(entry -> { + // Emit a non-loadable row, since all the loadable rows showed up in the previous iteration. + // Two ManifestEntries may produce the same row if they have different URIs + rows.add(newRow(source, entry.className, entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests, aliases)); + })); + return rows; + } - private static void handlePlugin( - // fixed - Config config, - // each location - PluginSource source, - Map> aliases, - Map> manifests, - // each plugin - String pluginName, - String pluginVersion, - PluginType pluginType, - boolean loadable - ) { - handlePlugin(config, source, pluginName, pluginVersion, aliases.getOrDefault(pluginName, Collections.emptyList()), pluginType, loadable, manifests.containsKey(pluginName)); + private static Row newRow(PluginSource source, String className, PluginType type, String version, boolean loadable, Map> manifests, Map> aliases) { + List rowAliases = aliases.getOrDefault(className, Collections.emptyList()); + boolean hasManifest = manifests.containsKey(className); + return new Row(source, className, type, version, rowAliases, loadable, hasManifest); } - private static void handlePlugin( - // fixed + private static void runCommand( Config config, - // each location - PluginSource source, - // each plugin - String pluginName, - String pluginVersion, - List aliases, - PluginType pluginType, - boolean loadable, - boolean hasManifest + Map>> manifests, + Map plugins, + Map> rows ) { - Path pluginLocation = source.location(); + beginCommand(config); + for (Map.Entry> entry : rows.entrySet()) { + for (Row row : entry.getValue()) { + handlePlugin(config, row); + } + } + endCommand(config, manifests, plugins, rows); + } + + private static void beginCommand(Config config) { + if (config.command == Command.LIST) { + listTablePrint(config, LIST_TABLE_COLUMN_COUNT, + "pluginName", + "firstAlias", + "secondAlias", + "pluginVersion", + "pluginType", + "isLoadable", + "hasManifest", + "pluginLocation" // last because it is least important and most repetitive + ); + } + } + + private static void handlePlugin(Config config, Row row) { + Path pluginLocation = row.source.location(); if (config.command == Command.LIST) { - String firstAlias = aliases.size() > 0 ? aliases.get(0) : "null"; - String secondAlias = aliases.size() > 1 ? aliases.get(1) : "null"; - String pluginInfo = Stream.of( - pluginName, + String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : "null"; + String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : "null"; + listTablePrint(config, LIST_TABLE_COLUMN_COUNT, + row.className, firstAlias, secondAlias, - pluginVersion, - pluginType, - loadable, - hasManifest, + row.version, + row.type, + row.loadable, + row.hasManifest, pluginLocation // last because it is least important and most repetitive - ).map(Objects::toString).collect(Collectors.joining("\t")); - config.out.println(pluginInfo); + ); } } - private static void endCommand(Config config) { + private static void endCommand( + Config config, + Map>> manifests, + Map plugins, + Map> rows) { + if (config.command == Command.LIST) { + // end the table with an empty line + config.out.println(); + listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, + "totalPlugins", // Total units of work + "loadable", + "hasManifest", + "pluginLocation" + ); + for (Map.Entry> entry : rows.entrySet()) { + PluginSource source = entry.getKey(); + int totalCount = entry.getValue().size(); + AtomicInteger pluginCount = new AtomicInteger(); + plugins.get(source).forEach(desc -> pluginCount.incrementAndGet()); + int manifestCount = 0; + for (Set manifestEntries : manifests.get(source).values()) { + manifestCount += manifestEntries.stream() + // If a plugin has multiple ManifestEntries for a single type, ignore the duplicates. + .map(manifestEntry -> manifestEntry.type) + .collect(Collectors.toSet()).size(); + } + listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, + totalCount, + pluginCount, + manifestCount, + source.location() + ); + } + } + } + private static void listTablePrint(Config config, int length, Object... args) { + if (length != args.length) { + throw new IllegalArgumentException("Table must have exactly " + length + " columns"); + } + config.out.println(Stream.of(args) + .map(Objects::toString) + .collect(Collectors.joining("\t"))); } - private static PluginScanResult discoverPlugins(PluginSource source) { - PluginScanResult serviceLoadResult = new ServiceLoaderScanner().discoverPlugins(Collections.singleton(source)); - PluginScanResult reflectiveResult = new ReflectionScanner().discoverPlugins(Collections.singleton(source)); + private static PluginScanResult discoverPlugins(PluginSource source, ReflectionScanner reflectionScanner, ServiceLoaderScanner serviceLoaderScanner) { + PluginScanResult serviceLoadResult = serviceLoaderScanner.discoverPlugins(Collections.singleton(source)); + PluginScanResult reflectiveResult = reflectionScanner.discoverPlugins(Collections.singleton(source)); return new PluginScanResult(Arrays.asList(serviceLoadResult, reflectiveResult)); } From eff32471fb93ad6789fee4bb611beaaf1fdf765d Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Tue, 1 Aug 2023 11:31:03 -0700 Subject: [PATCH 07/28] fixup: add NonMigrated TestPlugins, fix NPE in enumerateRows Signed-off-by: Greg Harris --- .../runtime/isolation/TestPlugins.java | 10 ++- .../test/plugins/NonMigratedConverter.java | 46 ++++++++++ .../plugins/NonMigratedHeaderConverter.java | 57 ++++++++++++ .../test/plugins/NonMigratedMultiPlugin.java | 87 +++++++++++++++++++ .../test/plugins/NonMigratedPredicate.java | 51 +++++++++++ .../plugins/NonMigratedSinkConnector.java | 61 +++++++++++++ .../plugins/NonMigratedSourceConnector.java | 61 +++++++++++++ .../plugins/NonMigratedTransformation.java | 50 +++++++++++ .../apache/kafka/tools/ConnectPluginPath.java | 2 +- .../kafka/tools/ConnectPluginPathTest.java | 12 +-- 10 files changed, 429 insertions(+), 8 deletions(-) create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedConverter.java create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedHeaderConverter.java create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedPredicate.java create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSinkConnector.java create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSourceConnector.java create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedTransformation.java diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java index d93fef181aa8d..5cf030c693b8c 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java @@ -173,7 +173,15 @@ public enum TestPlugin { /** * A ServiceLoader discovered plugin which subclasses another plugin which is present on the classpath */ - SUBCLASS_OF_CLASSPATH_OVERRIDE_POLICY("subclass-of-classpath", "test.plugins.SubclassOfClasspathOverridePolicy"); + SUBCLASS_OF_CLASSPATH_OVERRIDE_POLICY("subclass-of-classpath", "test.plugins.SubclassOfClasspathOverridePolicy"), + NON_MIGRATED_CONVERTER("non-migrated", "test.plugins.NonMigratedConverter", false), + NON_MIGRATED_HEADER_CONVERTER("non-migrated", "test.plugins.NonMigratedHeaderConverter", false), + NON_MIGRATED_MULTI_PLUGIN("non-migrated", "test.plugins.NonMigratedMultiPlugin", false), + NON_MIGRATED_PREDICATE("non-migrated", "test.plugins.NonMigratedPredicate", false), + NON_MIGRATED_SINK_CONNECTOR("non-migrated", "test.plugins.NonMigratedSinkConnector", false), + NON_MIGRATED_SOURCE_CONNECTOR("non-migrated", "test.plugins.NonMigratedSourceConnector", false), + NON_MIGRATED_TRANSFORMATION("non-migrated", "test.plugins.NonMigratedTransformation", false) + ; private final String resourceDir; private final String className; diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedConverter.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedConverter.java new file mode 100644 index 0000000000000..d2be39384ad13 --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedConverter.java @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import org.apache.kafka.connect.data.Schema; +import org.apache.kafka.connect.data.SchemaAndValue; +import org.apache.kafka.connect.storage.Converter; + +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public final class NonMigratedConverter implements Converter { + + @Override + public void configure(final Map configs, final boolean isKey) { + } + + @Override + public byte[] fromConnectData(final String topic, final Schema schema, final Object value) { + return new byte[0]; + } + + @Override + public SchemaAndValue toConnectData(final String topic, final byte[] value) { + return null; + } +} diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedHeaderConverter.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedHeaderConverter.java new file mode 100644 index 0000000000000..9e013303a5696 --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedHeaderConverter.java @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.connect.data.Schema; +import org.apache.kafka.connect.data.SchemaAndValue; +import org.apache.kafka.connect.storage.HeaderConverter; + +import java.io.IOException; +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public class NonMigratedHeaderConverter implements HeaderConverter { + + @Override + public SchemaAndValue toConnectHeader(String topic, String headerKey, byte[] value) { + return null; + } + + @Override + public byte[] fromConnectHeader(String topic, String headerKey, Schema schema, Object value) { + return new byte[0]; + } + + @Override + public ConfigDef config() { + return null; + } + + @Override + public void close() throws IOException { + } + + @Override + public void configure(Map configs) { + } +} diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java new file mode 100644 index 0000000000000..6d335cf6481dc --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.connect.connector.ConnectRecord; +import org.apache.kafka.connect.data.Schema; +import org.apache.kafka.connect.data.SchemaAndValue; +import org.apache.kafka.connect.storage.Converter; +import org.apache.kafka.connect.storage.HeaderConverter; +import org.apache.kafka.connect.transforms.predicates.Predicate; +import org.apache.kafka.connect.transforms.Transformation; + +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public final class NonMigratedMultiPlugin implements Converter, HeaderConverter, Predicate, Transformation { + + @Override + public void configure(Map configs, boolean isKey) { + + } + + @Override + public byte[] fromConnectData(String topic, Schema schema, Object value) { + return new byte[0]; + } + + @Override + public SchemaAndValue toConnectData(String topic, byte[] value) { + return null; + } + + @Override + public SchemaAndValue toConnectHeader(String topic, String headerKey, byte[] value) { + return null; + } + + @Override + public byte[] fromConnectHeader(String topic, String headerKey, Schema schema, Object value) { + return new byte[0]; + } + + @Override + public ConnectRecord apply(ConnectRecord record) { + return null; + } + + @Override + public ConfigDef config() { + return null; + } + + @Override + public boolean test(ConnectRecord record) { + return false; + } + + @Override + public void close() { + + } + + @Override + public void configure(Map configs) { + + } +} diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedPredicate.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedPredicate.java new file mode 100644 index 0000000000000..3465d0b4633c9 --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedPredicate.java @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + + +import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.connect.connector.ConnectRecord; +import org.apache.kafka.connect.transforms.predicates.Predicate; + +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public class NonMigratedPredicate implements Predicate { + + @Override + public void configure(Map configs) { + } + + @Override + public ConfigDef config() { + return null; + } + + @Override + public boolean test(ConnectRecord record) { + return false; + } + + @Override + public void close() { + } +} diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSinkConnector.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSinkConnector.java new file mode 100644 index 0000000000000..b9678a3382f23 --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSinkConnector.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.connect.connector.Task; +import org.apache.kafka.connect.sink.SinkConnector; + +import java.util.List; +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public class NonMigratedSinkConnector extends SinkConnector { + + @Override + public void start(Map props) { + } + + @Override + public Class taskClass() { + return null; + } + + @Override + public List> taskConfigs(int maxTasks) { + return null; + } + + @Override + public void stop() { + } + + @Override + public ConfigDef config() { + return null; + } + + @Override + public String version() { + return "1.0.0"; + } +} diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSourceConnector.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSourceConnector.java new file mode 100644 index 0000000000000..edbba6df3fedb --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedSourceConnector.java @@ -0,0 +1,61 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.connect.connector.Task; +import org.apache.kafka.connect.source.SourceConnector; + +import java.util.List; +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public class NonMigratedSourceConnector extends SourceConnector { + + @Override + public void start(Map props) { + } + + @Override + public Class taskClass() { + return null; + } + + @Override + public List> taskConfigs(int maxTasks) { + return null; + } + + @Override + public void stop() { + } + + @Override + public ConfigDef config() { + return null; + } + + @Override + public String version() { + return "1.0.0"; + } +} diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedTransformation.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedTransformation.java new file mode 100644 index 0000000000000..c3a732568f7dc --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedTransformation.java @@ -0,0 +1,50 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package test.plugins; + +import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.connect.connector.ConnectRecord; +import org.apache.kafka.connect.transforms.Transformation; + +import java.util.Map; + +/** + * Fake plugin class for testing classloading isolation. + * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. + *

Class which is not migrated to include a service loader manifest. + */ +public class NonMigratedTransformation implements Transformation { + + @Override + public void configure(Map configs) { + } + + @Override + public ConnectRecord apply(ConnectRecord record) { + return null; + } + + @Override + public ConfigDef config() { + return null; + } + + @Override + public void close() { + } +} diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 5881ea55aca40..9b6f171957d41 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -286,7 +286,7 @@ private static Set enumerateRows(PluginSource source, Map entry.type == pluginDesc.type()); + unloadablePlugins.getOrDefault(pluginDesc.className(), Collections.emptySet()).removeIf(entry -> entry.type == pluginDesc.type()); }); unloadablePlugins.values().forEach(entries -> entries.forEach(entry -> { // Emit a non-loadable row, since all the loadable rows showed up in the previous iteration. diff --git a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java index e7b1402f52a34..4ca91bba04371 100644 --- a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java +++ b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java @@ -299,7 +299,7 @@ public void testListOneLocation(PluginLocationType type) { CommandResult res = runCommand( "list", "--plugin-location", - setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.SAMPLING_CONVERTER) + setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN) ); assertEquals(0, res.returnCode); } @@ -310,7 +310,7 @@ public void testListMultipleLocations(PluginLocationType type) { CommandResult res = runCommand( "list", "--plugin-location", - setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.SAMPLING_CONVERTER), + setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN), "--plugin-location", setupLocation(workspace.resolve("location-b"), type, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) ); @@ -324,7 +324,7 @@ public void testListOnePluginPath(PluginLocationType type) { "list", "--plugin-path", setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.SAMPLING_CONVERTER, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) ); assertEquals(0, res.returnCode); } @@ -336,7 +336,7 @@ public void testListMultiplePluginPaths(PluginLocationType type) { "list", "--plugin-path", setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.SAMPLING_CONVERTER, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE), + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE), "--plugin-path", setupPluginPathElement(workspace.resolve("path-b"), type, TestPlugins.TestPlugin.SAMPLING_HEADER_CONVERTER, TestPlugins.TestPlugin.ALIASED_STATIC_FIELD) @@ -352,7 +352,7 @@ public void testListOneWorkerConfig(PluginLocationType type) { "--worker-config", setupWorkerConfig(workspace.resolve("worker.properties"), setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.SAMPLING_CONVERTER)) + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN)) ); assertEquals(0, res.returnCode); } @@ -365,7 +365,7 @@ public void testListMultipleWorkerConfigs(PluginLocationType type) { "--worker-config", setupWorkerConfig(workspace.resolve("worker-a.properties"), setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.SAMPLING_CONVERTER)), + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN)), "--worker-config", setupWorkerConfig(workspace.resolve("worker-b.properties"), setupPluginPathElement(workspace.resolve("path-b"), type, From 913589e59c1ec9e88f954ad675aa2e90764a35da Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Tue, 1 Aug 2023 13:19:30 -0700 Subject: [PATCH 08/28] fixup: resolve OOM by preventing leaks of PluginSource objects via Row Signed-off-by: Greg Harris --- .../runtime/isolation/PluginUtils.java | 52 +++--- .../apache/kafka/tools/ConnectPluginPath.java | 151 ++++++++---------- .../kafka/tools/ConnectPluginPathTest.java | 2 +- 3 files changed, 95 insertions(+), 110 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index 5d7ea63c2bc26..f799eaccd731c 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -342,35 +342,34 @@ public static List pluginUrls(Path topPath) throws IOException { } public static Set pluginSources(Set pluginLocations, ClassLoader classLoader, PluginClassLoaderFactory factory) { - Set pluginSources = new HashSet<>(isolatedPluginSources(pluginLocations, classLoader, factory)); - pluginSources.add(classpathPluginSource(classLoader.getParent())); - return pluginSources; - } - - public static Set isolatedPluginSources(Set pluginLocations, ClassLoader parent, PluginClassLoaderFactory factory) { Set pluginSources = new LinkedHashSet<>(); for (Path pluginLocation : pluginLocations) { try { - List pluginUrls = new ArrayList<>(); - for (Path path : pluginUrls(pluginLocation)) { - pluginUrls.add(path.toUri().toURL()); - } - URL[] urls = pluginUrls.toArray(new URL[0]); - PluginClassLoader loader = factory.newPluginClassLoader( - pluginLocation.toUri().toURL(), - urls, - parent - ); - pluginSources.add(new PluginSource(pluginLocation, loader, urls)); + pluginSources.add(isolatedPluginSource(pluginLocation, classLoader, factory)); } catch (InvalidPathException | MalformedURLException e) { log.error("Invalid path in plugin path: {}. Ignoring.", pluginLocation, e); } catch (IOException e) { log.error("Could not get listing for plugin path: {}. Ignoring.", pluginLocation, e); } } + pluginSources.add(classpathPluginSource(classLoader.getParent())); return pluginSources; } + public static PluginSource isolatedPluginSource(Path pluginLocation, ClassLoader parent, PluginClassLoaderFactory factory) throws IOException { + List pluginUrls = new ArrayList<>(); + for (Path path : pluginUrls(pluginLocation)) { + pluginUrls.add(path.toUri().toURL()); + } + URL[] urls = pluginUrls.toArray(new URL[0]); + PluginClassLoader loader = factory.newPluginClassLoader( + pluginLocation.toUri().toURL(), + urls, + parent + ); + return new PluginSource(pluginLocation, loader, urls); + } + public static PluginSource classpathPluginSource(ClassLoader classLoader) { List parentUrls = new ArrayList<>(); parentUrls.addAll(ClasspathHelper.forJavaClassPath()); @@ -388,6 +387,11 @@ public static String simpleName(PluginDesc plugin) { return plugin.pluginClass().getSimpleName(); } + public static String simpleName(String fullClassName) { + return fullClassName.substring(fullClassName.lastIndexOf('.') + 1); + } + + /** * Remove the plugin type name at the end of a plugin class name, if such suffix is present. * This method is meant to be used to extract plugin aliases. @@ -396,18 +400,22 @@ public static String simpleName(PluginDesc plugin) { * @return the pruned simple class name of the plugin. */ public static String prunedName(PluginDesc plugin) { + return prunedName(plugin.className(), plugin.type()); + } + + public static String prunedName(String fullClassName, PluginType type) { // It's currently simpler to switch on type than do pattern matching. - switch (plugin.type()) { + switch (type) { case SOURCE: case SINK: - return prunePluginName(plugin, "Connector"); + return prunePluginName(fullClassName, "Connector"); default: - return prunePluginName(plugin, plugin.type().simpleName()); + return prunePluginName(fullClassName, type.simpleName()); } } - private static String prunePluginName(PluginDesc plugin, String suffix) { - String simple = plugin.pluginClass().getSimpleName(); + private static String prunePluginName(String fullClassName, String suffix) { + String simple = simpleName(fullClassName); int pos = simple.lastIndexOf(suffix); if (pos > 0) { return simple.substring(0, pos); diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 9b6f171957d41..a9779356ad5eb 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -50,19 +50,17 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Properties; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -70,7 +68,7 @@ public class ConnectPluginPath { private static final String MANIFEST_PREFIX = "META-INF/services/"; private static final int LIST_TABLE_COLUMN_COUNT = 8; - private static final int LIST_SUMMARY_COLUMN_COUNT = 4; + private static final int LIST_SUMMARY_COLUMN_COUNT = 6; public static void main(String[] args) { Exit.exit(mainNoExit(args, System.out, System.err)); @@ -218,21 +216,19 @@ public static void runCommand(Config config) throws TerseException { ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { - Set isolatedSources = PluginUtils.isolatedPluginSources(config.locations, delegatingClassLoader, factory); - Map>> isolatedManifests = isolatedSources.stream() - .collect(Collectors.toMap(Function.identity(), - // Exclude manifest entries which are visible from the classpath - source -> findManifests(source, classpathManifests))); - Map isolatedPlugins = isolatedSources.stream() - .collect(Collectors.toMap(Function.identity(), - source -> discoverPlugins(source, reflectionScanner, serviceLoaderScanner))); - // Calculate aliases including collisions with classpath plugins - Map> aliases = computeAliases(classpathPlugins, isolatedPlugins.values()); - // Merge the plugins and manifest results to get the individual units of work - Map> isolatedRows = isolatedSources.stream() - .collect(Collectors.toMap(Function.identity(), - source -> enumerateRows(source, isolatedManifests.get(source), aliases, isolatedPlugins.get(source)))); - runCommand(config, isolatedManifests, isolatedPlugins, isolatedRows); + beginCommand(config); + Map> isolatedRows = new LinkedHashMap<>(); + for (Path pluginLocation : config.locations) { + PluginSource source = PluginUtils.isolatedPluginSource(pluginLocation, delegatingClassLoader, factory); + Map> manifests = findManifests(source, classpathManifests); + PluginScanResult plugins = discoverPlugins(source, reflectionScanner, serviceLoaderScanner); + Set rows = enumerateRows(source, manifests, plugins); + isolatedRows.put(pluginLocation, rows); + for (Row row : rows) { + handlePlugin(config, row); + } + } + endCommand(config, isolatedRows); } } catch (IOException e) { throw new UncheckedIOException(e); @@ -245,7 +241,7 @@ public static void runCommand(Config config) throws TerseException { * that pertains to this specific plugin. */ private static class Row { - private final PluginSource source; + private final Path pluginLocation; private final String className; private final PluginType type; private final String version; @@ -253,8 +249,8 @@ private static class Row { private final boolean loadable; private final boolean hasManifest; - public Row(PluginSource source, String className, PluginType type, String version, List aliases, boolean loadable, boolean hasManifest) { - this.source = source; + public Row(Path pluginLocation, String className, PluginType type, String version, List aliases, boolean loadable, boolean hasManifest) { + this.pluginLocation = pluginLocation; this.className = className; this.version = version; this.type = type; @@ -263,58 +259,57 @@ public Row(PluginSource source, String className, PluginType type, String versio this.hasManifest = hasManifest; } + private boolean loadable() { + return loadable; + } + + private boolean hasManifest() { + return hasManifest; + } + + private boolean compatible() { + return loadable && hasManifest; + } + @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; Row row = (Row) o; - return source.equals(row.source) && className.equals(row.className) && type == row.type; + return pluginLocation.equals(row.pluginLocation) && className.equals(row.className) && type == row.type; } @Override public int hashCode() { - return Objects.hash(source, className, type); + return Objects.hash(pluginLocation, className, type); } } - private static Set enumerateRows(PluginSource source, Map> manifests, Map> aliases, PluginScanResult scanResult) { + private static Set enumerateRows(PluginSource source, Map> manifests, PluginScanResult scanResult) { Set rows = new HashSet<>(); // Perform a deep copy of the manifests because we're going to be mutating our copy. Map> unloadablePlugins = manifests.entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, e -> new HashSet<>(e.getValue()))); scanResult.forEach(pluginDesc -> { // Emit a loadable row for this scan result, since it was found during plugin discovery - rows.add(newRow(source, pluginDesc.className(), pluginDesc.type(), pluginDesc.version(), true, manifests, aliases)); + rows.add(newRow(source, pluginDesc.className(), pluginDesc.type(), pluginDesc.version(), true, manifests)); // Remove the ManifestEntry if it has the same className and type as one of the loadable plugins. unloadablePlugins.getOrDefault(pluginDesc.className(), Collections.emptySet()).removeIf(entry -> entry.type == pluginDesc.type()); }); unloadablePlugins.values().forEach(entries -> entries.forEach(entry -> { // Emit a non-loadable row, since all the loadable rows showed up in the previous iteration. // Two ManifestEntries may produce the same row if they have different URIs - rows.add(newRow(source, entry.className, entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests, aliases)); + rows.add(newRow(source, entry.className, entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests)); })); return rows; } - private static Row newRow(PluginSource source, String className, PluginType type, String version, boolean loadable, Map> manifests, Map> aliases) { - List rowAliases = aliases.getOrDefault(className, Collections.emptyList()); + private static Row newRow(PluginSource source, String className, PluginType type, String version, boolean loadable, Map> manifests) { + Set rowAliases = new LinkedHashSet<>(); + rowAliases.add(PluginUtils.simpleName(className)); + rowAliases.add(PluginUtils.prunedName(className, type)); boolean hasManifest = manifests.containsKey(className); - return new Row(source, className, type, version, rowAliases, loadable, hasManifest); - } - - private static void runCommand( - Config config, - Map>> manifests, - Map plugins, - Map> rows - ) { - beginCommand(config); - for (Map.Entry> entry : rows.entrySet()) { - for (Row row : entry.getValue()) { - handlePlugin(config, row); - } - } - endCommand(config, manifests, plugins, rows); + return new Row(source.location(), className, type, version, new ArrayList<>(rowAliases), loadable, hasManifest); } private static void beginCommand(Config config) { @@ -333,7 +328,6 @@ private static void beginCommand(Config config) { } private static void handlePlugin(Config config, Row row) { - Path pluginLocation = row.source.location(); if (config.command == Command.LIST) { String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : "null"; String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : "null"; @@ -345,44 +339,41 @@ private static void handlePlugin(Config config, Row row) { row.type, row.loadable, row.hasManifest, - pluginLocation // last because it is least important and most repetitive + row.pluginLocation // last because it is least important and most repetitive ); } } private static void endCommand( Config config, - Map>> manifests, - Map plugins, - Map> rows) { + Map> rowsByLocation + ) { if (config.command == Command.LIST) { // end the table with an empty line config.out.println(); listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, - "totalPlugins", // Total units of work - "loadable", - "hasManifest", - "pluginLocation" + "totalPlugins", + "loadablePlugins", + "compatiblePlugins", + "totalLocations", + "compatibleLocations", + "allLocationsCompatible" + ); + Set allRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); + long totalPlugins = allRows.size(); + long loadablePlugins = allRows.stream().filter(Row::loadable).count(); + long compatiblePlugins = allRows.stream().filter(Row::compatible).count(); + long totalLocations = rowsByLocation.size(); + long compatibleLocations = rowsByLocation.values().stream().filter(location -> location.stream().allMatch(Row::compatible)).count(); + boolean allLocationsCompatible = allRows.stream().allMatch(Row::compatible); + listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, + totalPlugins, + loadablePlugins, + compatiblePlugins, + totalLocations, + compatibleLocations, + allLocationsCompatible ); - for (Map.Entry> entry : rows.entrySet()) { - PluginSource source = entry.getKey(); - int totalCount = entry.getValue().size(); - AtomicInteger pluginCount = new AtomicInteger(); - plugins.get(source).forEach(desc -> pluginCount.incrementAndGet()); - int manifestCount = 0; - for (Set manifestEntries : manifests.get(source).values()) { - manifestCount += manifestEntries.stream() - // If a plugin has multiple ManifestEntries for a single type, ignore the duplicates. - .map(manifestEntry -> manifestEntry.type) - .collect(Collectors.toSet()).size(); - } - listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, - totalCount, - pluginCount, - manifestCount, - source.location() - ); - } } } @@ -495,18 +486,4 @@ private static int parseLine(BufferedReader r, int lc, Set names) throws } return lc + 1; } - - private static Map> computeAliases(PluginScanResult classpathPlugins, Collection isolatedPlugins) { - List allResults = new ArrayList<>(isolatedPlugins); - allResults.add(classpathPlugins); - return PluginUtils.computeAliases(new PluginScanResult(allResults)).entrySet() - .stream() - .collect(Collectors.toMap( - Map.Entry::getValue, - e -> new ArrayList<>(Collections.singletonList(e.getKey())), - (a, b) -> { - a.addAll(b); - return a; - })); - } } diff --git a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java index 4ca91bba04371..7440de59f32f2 100644 --- a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java +++ b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java @@ -231,7 +231,7 @@ private CommandResult runCommand(Object... args) { ClassLoader parent = ConnectPluginPath.class.getClassLoader(); ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { - Set sources = PluginUtils.isolatedPluginSources(pluginLocations, delegatingClassLoader, factory); + Set sources = PluginUtils.pluginSources(pluginLocations, delegatingClassLoader, factory); String stdout = new String(out.toByteArray(), StandardCharsets.UTF_8); String stderr = new String(err.toByteArray(), StandardCharsets.UTF_8); log.info("STDOUT:\n{}", stdout); From 2a8dfe9c9b3d2e56a81c122dc08eef4ee6b3efe6 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Tue, 1 Aug 2023 14:14:02 -0700 Subject: [PATCH 09/28] fixup: change summary format to a column-oriented table Signed-off-by: Greg Harris --- .../runtime/isolation/PluginUtils.java | 4 + .../apache/kafka/tools/ConnectPluginPath.java | 75 +++++++++++-------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index f799eaccd731c..a8fc4fca1f226 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -16,6 +16,7 @@ */ package org.apache.kafka.connect.runtime.isolation; +import org.apache.commons.lang3.tuple.Pair; import org.reflections.util.ClasspathHelper; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -43,6 +44,9 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.function.BiConsumer; +import java.util.function.Consumer; +import java.util.function.Supplier; import java.util.regex.Pattern; /** diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index a9779356ad5eb..cf03ad7a8370a 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -68,7 +68,6 @@ public class ConnectPluginPath { private static final String MANIFEST_PREFIX = "META-INF/services/"; private static final int LIST_TABLE_COLUMN_COUNT = 8; - private static final int LIST_SUMMARY_COLUMN_COUNT = 6; public static void main(String[] args) { Exit.exit(mainNoExit(args, System.out, System.err)); @@ -213,22 +212,24 @@ public static void runCommand(Config config) throws TerseException { PluginSource classpathSource = PluginUtils.classpathPluginSource(parent); Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); PluginScanResult classpathPlugins = discoverPlugins(classpathSource, reflectionScanner, serviceLoaderScanner); + Map> rowsByLocation = new LinkedHashMap<>(); + Set classpathRows = enumerateRows(classpathSource, classpathManifests, classpathPlugins); + rowsByLocation.put(classpathSource.location(), classpathRows); ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { beginCommand(config); - Map> isolatedRows = new LinkedHashMap<>(); for (Path pluginLocation : config.locations) { PluginSource source = PluginUtils.isolatedPluginSource(pluginLocation, delegatingClassLoader, factory); Map> manifests = findManifests(source, classpathManifests); PluginScanResult plugins = discoverPlugins(source, reflectionScanner, serviceLoaderScanner); Set rows = enumerateRows(source, manifests, plugins); - isolatedRows.put(pluginLocation, rows); + rowsByLocation.put(pluginLocation, rows); for (Row row : rows) { handlePlugin(config, row); } } - endCommand(config, isolatedRows); + endCommand(config, rowsByLocation); } } catch (IOException e) { throw new UncheckedIOException(e); @@ -263,10 +264,6 @@ private boolean loadable() { return loadable; } - private boolean hasManifest() { - return hasManifest; - } - private boolean compatible() { return loadable && hasManifest; } @@ -314,7 +311,7 @@ private static Row newRow(PluginSource source, String className, PluginType type private static void beginCommand(Config config) { if (config.command == Command.LIST) { - listTablePrint(config, LIST_TABLE_COLUMN_COUNT, + listTablePrint(config, "pluginName", "firstAlias", "secondAlias", @@ -331,7 +328,7 @@ private static void handlePlugin(Config config, Row row) { if (config.command == Command.LIST) { String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : "null"; String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : "null"; - listTablePrint(config, LIST_TABLE_COLUMN_COUNT, + listTablePrint(config, row.className, firstAlias, secondAlias, @@ -351,35 +348,37 @@ private static void endCommand( if (config.command == Command.LIST) { // end the table with an empty line config.out.println(); - listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, - "totalPlugins", - "loadablePlugins", - "compatiblePlugins", - "totalLocations", - "compatibleLocations", - "allLocationsCompatible" - ); Set allRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); - long totalPlugins = allRows.size(); - long loadablePlugins = allRows.stream().filter(Row::loadable).count(); - long compatiblePlugins = allRows.stream().filter(Row::compatible).count(); + Map> aliasCollisions = aliasCollisions(allRows); + for (Map.Entry> entry : aliasCollisions.entrySet()) { + String alias = entry.getKey(); + Set classNames = entry.getValue(); + if (classNames.size() != 1) { + config.out.printf("Ignoring ambiguous alias '%s' since it refers to multiple distinct plugins %s%n", + alias, classNames); + } + } + rowsByLocation.remove(PluginSource.CLASSPATH); + Set isolatedRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); + long totalPlugins = isolatedRows.size(); + long loadablePlugins = isolatedRows.stream().filter(Row::loadable).count(); + long compatiblePlugins = isolatedRows.stream().filter(Row::compatible).count(); long totalLocations = rowsByLocation.size(); long compatibleLocations = rowsByLocation.values().stream().filter(location -> location.stream().allMatch(Row::compatible)).count(); - boolean allLocationsCompatible = allRows.stream().allMatch(Row::compatible); - listTablePrint(config, LIST_SUMMARY_COLUMN_COUNT, - totalPlugins, - loadablePlugins, - compatiblePlugins, - totalLocations, - compatibleLocations, - allLocationsCompatible - ); + boolean allLocationsCompatible = isolatedRows.stream().allMatch(Row::compatible); + config.out.printf("Total plugins: \t%d%n", totalPlugins); + config.out.printf("Loadable plugins: \t%d%n", loadablePlugins); + config.out.printf("Compatible plugins: \t%d%n", compatiblePlugins); + config.out.printf("Total locations: \t%d%n", totalLocations); + config.out.printf("Compatible locations: \t%d%n", compatibleLocations); + config.out.printf("All locations compatible?\t%b%n", allLocationsCompatible); + } } - private static void listTablePrint(Config config, int length, Object... args) { - if (length != args.length) { - throw new IllegalArgumentException("Table must have exactly " + length + " columns"); + private static void listTablePrint(Config config, Object... args) { + if (ConnectPluginPath.LIST_TABLE_COLUMN_COUNT != args.length) { + throw new IllegalArgumentException("Table must have exactly " + ConnectPluginPath.LIST_TABLE_COLUMN_COUNT + " columns"); } config.out.println(Stream.of(args) .map(Objects::toString) @@ -486,4 +485,14 @@ private static int parseLine(BufferedReader r, int lc, Set names) throws } return lc + 1; } + + private static Map> aliasCollisions(Set rows) { + Map> aliasCollisions = new HashMap<>(); + rows.forEach(row -> { + aliasCollisions.computeIfAbsent(PluginUtils.simpleName(row.className), ignored -> new HashSet<>()).add(row.className); + aliasCollisions.computeIfAbsent(PluginUtils.prunedName(row.className, row.type), ignored -> new HashSet<>()).add(row.className); + }); + return aliasCollisions; + } + } From 4366dd648ab22d0eeb9eab665b847010b61b588f Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 2 Aug 2023 09:56:23 -0700 Subject: [PATCH 10/28] fixup: checkstyle Signed-off-by: Greg Harris --- .../apache/kafka/connect/runtime/isolation/TestPlugins.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java index 5cf030c693b8c..8f675a5a42e78 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java @@ -180,8 +180,7 @@ public enum TestPlugin { NON_MIGRATED_PREDICATE("non-migrated", "test.plugins.NonMigratedPredicate", false), NON_MIGRATED_SINK_CONNECTOR("non-migrated", "test.plugins.NonMigratedSinkConnector", false), NON_MIGRATED_SOURCE_CONNECTOR("non-migrated", "test.plugins.NonMigratedSourceConnector", false), - NON_MIGRATED_TRANSFORMATION("non-migrated", "test.plugins.NonMigratedTransformation", false) - ; + NON_MIGRATED_TRANSFORMATION("non-migrated", "test.plugins.NonMigratedTransformation", false); private final String resourceDir; private final String className; From 26330a7874cb22c29ea97ba323a67a1ee0af3aa0 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 2 Aug 2023 10:10:23 -0700 Subject: [PATCH 11/28] fixup: checkstyle Signed-off-by: Greg Harris --- .../apache/kafka/connect/runtime/isolation/PluginUtils.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index a8fc4fca1f226..f799eaccd731c 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -16,7 +16,6 @@ */ package org.apache.kafka.connect.runtime.isolation; -import org.apache.commons.lang3.tuple.Pair; import org.reflections.util.ClasspathHelper; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -44,9 +43,6 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; -import java.util.function.BiConsumer; -import java.util.function.Consumer; -import java.util.function.Supplier; import java.util.regex.Pattern; /** From 85321f575286f27528d1eeafe312844c90da582f Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 2 Aug 2023 12:27:24 -0700 Subject: [PATCH 12/28] fixup: review comments Signed-off-by: Greg Harris --- .../runtime/isolation/PluginUtils.java | 8 +-- .../apache/kafka/tools/ConnectPluginPath.java | 58 ++++++++++--------- 2 files changed, 35 insertions(+), 31 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index f799eaccd731c..2b2b3fbc779c8 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -206,11 +206,9 @@ public static Set pluginLocations(String pluginPath, boolean failFast) { Set pluginLocations = new LinkedHashSet<>(); for (String path : pluginPathElements) { try { - Path specifiedPath = Paths.get(path); - Path pluginPathElement = specifiedPath.toAbsolutePath(); - if (!specifiedPath.isAbsolute()) { - log.warn("Plugin path element '{}' is relative, evaluating to {}.", - specifiedPath, pluginPathElement); + Path pluginPathElement = Paths.get(path).toAbsolutePath(); + if (!pluginPath.isEmpty()) { + log.warn("Plugin path element is empty, evaluating to {}.", pluginPathElement); } if (!Files.exists(pluginPathElement)) { throw new FileNotFoundException(pluginPathElement.toString()); diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index cf03ad7a8370a..1371f21c3fd0c 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -67,7 +67,16 @@ public class ConnectPluginPath { private static final String MANIFEST_PREFIX = "META-INF/services/"; - private static final int LIST_TABLE_COLUMN_COUNT = 8; + private static final Object[] LIST_TABLE_COLUMNS = { + "pluginName", + "firstAlias", + "secondAlias", + "pluginVersion", + "pluginType", + "isLoadable", + "hasManifest", + "pluginLocation" // last because it is least important and most repetitive + }; public static void main(String[] args) { Exit.exit(mainNoExit(args, System.out, System.err)); @@ -311,23 +320,17 @@ private static Row newRow(PluginSource source, String className, PluginType type private static void beginCommand(Config config) { if (config.command == Command.LIST) { - listTablePrint(config, - "pluginName", - "firstAlias", - "secondAlias", - "pluginVersion", - "pluginType", - "isLoadable", - "hasManifest", - "pluginLocation" // last because it is least important and most repetitive - ); + // The list command prints a TSV-formatted table with details of the found plugins + // This is officially human-readable output with no guarantees for backwards-compatibility + // It should be reasonably easy to parse for ad-hoc scripting use-cases. + listTablePrint(config, LIST_TABLE_COLUMNS); } } private static void handlePlugin(Config config, Row row) { if (config.command == Command.LIST) { - String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : "null"; - String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : "null"; + String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : "N/A"; + String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : "N/A"; listTablePrint(config, row.className, firstAlias, @@ -346,7 +349,7 @@ private static void endCommand( Map> rowsByLocation ) { if (config.command == Command.LIST) { - // end the table with an empty line + // end the table with an empty line to enable users to separate the table from the summary. config.out.println(); Set allRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); Map> aliasCollisions = aliasCollisions(allRows); @@ -354,8 +357,10 @@ private static void endCommand( String alias = entry.getKey(); Set classNames = entry.getValue(); if (classNames.size() != 1) { - config.out.printf("Ignoring ambiguous alias '%s' since it refers to multiple distinct plugins %s%n", - alias, classNames); + config.out.printf("%s is an ambiguous alias and will not be usable. One of the fully qualified classes must be used instead:%n", alias); + for (String className : classNames) { + config.out.printf("\t%s%n", className); + } } } rowsByLocation.remove(PluginSource.CLASSPATH); @@ -377,8 +382,8 @@ private static void endCommand( } private static void listTablePrint(Config config, Object... args) { - if (ConnectPluginPath.LIST_TABLE_COLUMN_COUNT != args.length) { - throw new IllegalArgumentException("Table must have exactly " + ConnectPluginPath.LIST_TABLE_COLUMN_COUNT + " columns"); + if (ConnectPluginPath.LIST_TABLE_COLUMNS.length != args.length) { + throw new IllegalArgumentException("Table must have exactly " + ConnectPluginPath.LIST_TABLE_COLUMNS.length + " columns"); } config.out.println(Stream.of(args) .map(Objects::toString) @@ -439,9 +444,9 @@ private static Map> findManifests(PluginSource source return manifests; } - // Based on implementation from ServiceLoader.LazyClassPathLookupIterator + // Based on implementation from ServiceLoader.LazyClassPathLookupIterator from OpenJDK11 // visible for testing - static Set parse(URL u) { + private static Set parse(URL u) { Set names = new LinkedHashSet<>(); // preserve insertion order try { URLConnection uc = u.openConnection(); @@ -450,17 +455,18 @@ static Set parse(URL u) { BufferedReader r = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8))) { int lc = 1; - while ((lc = parseLine(r, lc, names)) >= 0) { + while ((lc = parseLine(u, r, lc, names)) >= 0) { // pass } } } catch (IOException x) { - throw new RuntimeException("Error accessing ServiceLoader manifest file " + u, x); + throw new RuntimeException("Error accessing configuration file", x); } return names; } - private static int parseLine(BufferedReader r, int lc, Set names) throws IOException { + // Based on implementation from ServiceLoader.LazyClassPathLookupIterator from OpenJDK11 + private static int parseLine(URL u, BufferedReader r, int lc, Set names) throws IOException { String ln = r.readLine(); if (ln == null) { return -1; @@ -471,15 +477,15 @@ private static int parseLine(BufferedReader r, int lc, Set names) throws int n = ln.length(); if (n != 0) { if ((ln.indexOf(' ') >= 0) || (ln.indexOf('\t') >= 0)) - throw new IOException("Illegal configuration-file syntax"); + throw new IOException("Illegal configuration-file syntax in " + u); int cp = ln.codePointAt(0); if (!Character.isJavaIdentifierStart(cp)) - throw new IOException("Illegal provider-class name: " + ln); + throw new IOException("Illegal provider-class name: " + ln + " in " + u); int start = Character.charCount(cp); for (int i = start; i < n; i += Character.charCount(cp)) { cp = ln.codePointAt(i); if (!Character.isJavaIdentifierPart(cp) && (cp != '.')) - throw new IOException("Illegal provider-class name: " + ln); + throw new IOException("Illegal provider-class name: " + ln + " in " + u); } names.add(ln); } From 643ea94e6a503f25e41d58d84d2cccfc71b07916 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 2 Aug 2023 13:07:44 -0700 Subject: [PATCH 13/28] fixup: fix URL uniqueness problem Signed-off-by: Greg Harris --- .../java/org/apache/kafka/tools/ConnectPluginPath.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 1371f21c3fd0c..34e7541b4ec1f 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -158,7 +158,7 @@ private static Set parseLocations(ArgumentParser parser, Namespace namespa if (rawLocations.isEmpty() && rawPluginPaths.isEmpty() && rawWorkerConfigs.isEmpty()) { throw new ArgumentParserException("Must specify at least one --plugin-location, --plugin-path, or --worker-config", parser); } - Set pluginLocations = new HashSet<>(); + Set pluginLocations = new LinkedHashSet<>(); for (String rawWorkerConfig : rawWorkerConfigs) { Properties properties; try { @@ -185,7 +185,10 @@ private static Set parseLocations(ArgumentParser parser, Namespace namespa } pluginLocations.add(pluginLocation); } - return pluginLocations; + // Inject a current directory reference in the path to make these plugin paths distinct from the classpath. + return pluginLocations.stream() + .map(path -> path.getParent().resolve(".").resolve(path.getFileName())) + .collect(Collectors.toSet()); } enum Command { From f1adfe3a82eedced5cb5262341ba1a23d7e0abba Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 2 Aug 2023 13:53:39 -0700 Subject: [PATCH 14/28] fixup: hide altered paths from table output Signed-off-by: Greg Harris --- .../apache/kafka/tools/ConnectPluginPath.java | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 34e7541b4ec1f..b34a58eb2b38f 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -185,10 +185,21 @@ private static Set parseLocations(ArgumentParser parser, Namespace namespa } pluginLocations.add(pluginLocation); } - // Inject a current directory reference in the path to make these plugin paths distinct from the classpath. - return pluginLocations.stream() - .map(path -> path.getParent().resolve(".").resolve(path.getFileName())) - .collect(Collectors.toSet()); + return pluginLocations; + } + + private static Path makeDistinct(Path path) { + // When the same jar is present on the classpath and inside a pluginLocation, we still need to be able + // to distinguish between them. In order to make them more likely to be distinct, change the path to + // include an arbitrary change that makes them still resolve the same file, but fail direct equality checks. + Path parent = path.getParent(); + if (parent == null) { + // At the root of the filesystem, add the current directory marker to the end. + return path.resolve("."); + } else { + // If we have a parent directory, add the current directory marker there. + return parent.resolve(".").resolve(path.getFileName()); + } } enum Command { @@ -225,17 +236,17 @@ public static void runCommand(Config config) throws TerseException { Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); PluginScanResult classpathPlugins = discoverPlugins(classpathSource, reflectionScanner, serviceLoaderScanner); Map> rowsByLocation = new LinkedHashMap<>(); - Set classpathRows = enumerateRows(classpathSource, classpathManifests, classpathPlugins); + Set classpathRows = enumerateRows(classpathSource.location(), classpathManifests, classpathPlugins); rowsByLocation.put(classpathSource.location(), classpathRows); ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { beginCommand(config); for (Path pluginLocation : config.locations) { - PluginSource source = PluginUtils.isolatedPluginSource(pluginLocation, delegatingClassLoader, factory); + PluginSource source = PluginUtils.isolatedPluginSource(makeDistinct(pluginLocation), delegatingClassLoader, factory); Map> manifests = findManifests(source, classpathManifests); PluginScanResult plugins = discoverPlugins(source, reflectionScanner, serviceLoaderScanner); - Set rows = enumerateRows(source, manifests, plugins); + Set rows = enumerateRows(pluginLocation, manifests, plugins); rowsByLocation.put(pluginLocation, rows); for (Row row : rows) { handlePlugin(config, row); @@ -294,31 +305,31 @@ public int hashCode() { } } - private static Set enumerateRows(PluginSource source, Map> manifests, PluginScanResult scanResult) { + private static Set enumerateRows(Path pluginLocation, Map> manifests, PluginScanResult scanResult) { Set rows = new HashSet<>(); // Perform a deep copy of the manifests because we're going to be mutating our copy. Map> unloadablePlugins = manifests.entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, e -> new HashSet<>(e.getValue()))); scanResult.forEach(pluginDesc -> { // Emit a loadable row for this scan result, since it was found during plugin discovery - rows.add(newRow(source, pluginDesc.className(), pluginDesc.type(), pluginDesc.version(), true, manifests)); + rows.add(newRow(pluginLocation, pluginDesc.className(), pluginDesc.type(), pluginDesc.version(), true, manifests)); // Remove the ManifestEntry if it has the same className and type as one of the loadable plugins. unloadablePlugins.getOrDefault(pluginDesc.className(), Collections.emptySet()).removeIf(entry -> entry.type == pluginDesc.type()); }); unloadablePlugins.values().forEach(entries -> entries.forEach(entry -> { // Emit a non-loadable row, since all the loadable rows showed up in the previous iteration. // Two ManifestEntries may produce the same row if they have different URIs - rows.add(newRow(source, entry.className, entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests)); + rows.add(newRow(pluginLocation, entry.className, entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests)); })); return rows; } - private static Row newRow(PluginSource source, String className, PluginType type, String version, boolean loadable, Map> manifests) { + private static Row newRow(Path pluginLocation, String className, PluginType type, String version, boolean loadable, Map> manifests) { Set rowAliases = new LinkedHashSet<>(); rowAliases.add(PluginUtils.simpleName(className)); rowAliases.add(PluginUtils.prunedName(className, type)); boolean hasManifest = manifests.containsKey(className); - return new Row(source.location(), className, type, version, new ArrayList<>(rowAliases), loadable, hasManifest); + return new Row(pluginLocation, className, type, version, new ArrayList<>(rowAliases), loadable, hasManifest); } private static void beginCommand(Config config) { From 3f06205677408024da50f8553e0fabab016b55f2 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 3 Aug 2023 09:56:18 -0700 Subject: [PATCH 15/28] fixup: false positive for multi-plugins Signed-off-by: Greg Harris --- .../org.apache.kafka.connect.storage.Converter | 16 ++++++++++++++++ .../apache/kafka/tools/ConnectPluginPath.java | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.storage.Converter diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.storage.Converter b/connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.storage.Converter new file mode 100644 index 0000000000000..82400f7255d56 --- /dev/null +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.storage.Converter @@ -0,0 +1,16 @@ + # Licensed to the Apache Software Foundation (ASF) under one or more + # contributor license agreements. See the NOTICE file distributed with + # this work for additional information regarding copyright ownership. + # The ASF licenses this file to You under the Apache License, Version 2.0 + # (the "License"); you may not use this file except in compliance with + # the License. You may obtain a copy of the License at + # + # http://www.apache.org/licenses/LICENSE-2.0 + # + # Unless required by applicable law or agreed to in writing, software + # distributed under the License is distributed on an "AS IS" BASIS, + # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + # See the License for the specific language governing permissions and + # limitations under the License. + +test.plugins.NonMigratedMultiPlugin diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index b34a58eb2b38f..85a59a142b963 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -328,7 +328,7 @@ private static Row newRow(Path pluginLocation, String className, PluginType type Set rowAliases = new LinkedHashSet<>(); rowAliases.add(PluginUtils.simpleName(className)); rowAliases.add(PluginUtils.prunedName(className, type)); - boolean hasManifest = manifests.containsKey(className); + boolean hasManifest = manifests.containsKey(className) && manifests.get(className).stream().anyMatch(e -> e.type == type); return new Row(pluginLocation, className, type, version, new ArrayList<>(rowAliases), loadable, hasManifest); } From a4044973815330dba5f1e8ed92dbe734690d926f Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 7 Aug 2023 12:21:39 -0700 Subject: [PATCH 16/28] fixup: typo inverted condition Signed-off-by: Greg Harris --- .../org/apache/kafka/connect/runtime/isolation/PluginUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index 2b2b3fbc779c8..d0b9bd6e178dc 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -207,7 +207,7 @@ public static Set pluginLocations(String pluginPath, boolean failFast) { for (String path : pluginPathElements) { try { Path pluginPathElement = Paths.get(path).toAbsolutePath(); - if (!pluginPath.isEmpty()) { + if (pluginPath.isEmpty()) { log.warn("Plugin path element is empty, evaluating to {}.", pluginPathElement); } if (!Files.exists(pluginPathElement)) { From 501159cee69007909d43336d19b9dbd5d7d4cf77 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 7 Aug 2023 14:23:03 -0700 Subject: [PATCH 17/28] fixup: increase heap available to connect-plugin-path command Signed-off-by: Greg Harris --- bin/connect-plugin-path.sh | 4 ++++ bin/windows/connect-plugin-path.bat | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/bin/connect-plugin-path.sh b/bin/connect-plugin-path.sh index b20ab76759143..50742061776fb 100755 --- a/bin/connect-plugin-path.sh +++ b/bin/connect-plugin-path.sh @@ -14,4 +14,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +if [ "x$KAFKA_HEAP_OPTS" = "x" ]; then + export KAFKA_HEAP_OPTS="-Xms256M -Xmx2G" +fi + exec $(dirname $0)/kafka-run-class.sh org.apache.kafka.tools.ConnectPluginPath "$@" diff --git a/bin/windows/connect-plugin-path.bat b/bin/windows/connect-plugin-path.bat index 35f16707693fc..3f64a8253e9f4 100644 --- a/bin/windows/connect-plugin-path.bat +++ b/bin/windows/connect-plugin-path.bat @@ -14,4 +14,8 @@ rem WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. rem See the License for the specific language governing permissions and rem limitations under the License. +IF ["%KAFKA_HEAP_OPTS%"] EQU [""] ( + set KAFKA_HEAP_OPTS=-Xms256M -Xmx2G +) + "%~dp0kafka-run-class.bat" org.apache.kafka.tools.ConnectPluginPath %* \ No newline at end of file From c477ee88bc4cfd4c4b542fad93b7a0ec3b1dd89a Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 7 Aug 2023 14:24:09 -0700 Subject: [PATCH 18/28] fixup: use null as classpath pluginLocation sentinel value, inject classpath string in row output Signed-off-by: Greg Harris --- .../runtime/isolation/PluginSource.java | 3 +-- .../runtime/isolation/PluginUtils.java | 2 +- .../apache/kafka/tools/ConnectPluginPath.java | 19 ++++++++++++------- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java index c7123d836613c..665785a1c536e 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java @@ -24,7 +24,6 @@ public class PluginSource { - public static final Path CLASSPATH = Paths.get("classpath"); private final Path location; private final ClassLoader loader; private final URL[] urls; @@ -48,7 +47,7 @@ public URL[] urls() { } public boolean isolated() { - return location != CLASSPATH; + return location != null; } @Override diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index d0b9bd6e178dc..4c340f5dfdfbd 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -372,7 +372,7 @@ public static PluginSource classpathPluginSource(ClassLoader classLoader) { List parentUrls = new ArrayList<>(); parentUrls.addAll(ClasspathHelper.forJavaClassPath()); parentUrls.addAll(ClasspathHelper.forClassLoader(classLoader)); - return new PluginSource(PluginSource.CLASSPATH, classLoader, parentUrls.toArray(new URL[0])); + return new PluginSource(null, classLoader, parentUrls.toArray(new URL[0])); } /** diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 85a59a142b963..6a888c3dcd6c7 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -275,10 +275,10 @@ private static class Row { public Row(Path pluginLocation, String className, PluginType type, String version, List aliases, boolean loadable, boolean hasManifest) { this.pluginLocation = pluginLocation; - this.className = className; - this.version = version; - this.type = type; - this.aliases = aliases; + this.className = Objects.requireNonNull(className, "className must be non-null"); + this.version = Objects.requireNonNull(version, "version must be non-null"); + this.type = Objects.requireNonNull(type, "type must be non-null"); + this.aliases = Objects.requireNonNull(aliases, "aliases must be non-null"); this.loadable = loadable; this.hasManifest = hasManifest; } @@ -291,12 +291,16 @@ private boolean compatible() { return loadable && hasManifest; } + private String locationString() { + return pluginLocation == null ? "classpath" : pluginLocation.toString(); + } + @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; Row row = (Row) o; - return pluginLocation.equals(row.pluginLocation) && className.equals(row.className) && type == row.type; + return Objects.equals(pluginLocation, row.pluginLocation) && className.equals(row.className) && type == row.type; } @Override @@ -353,7 +357,8 @@ private static void handlePlugin(Config config, Row row) { row.type, row.loadable, row.hasManifest, - row.pluginLocation // last because it is least important and most repetitive + // last because it is least important and most repetitive + row.locationString() ); } } @@ -377,7 +382,7 @@ private static void endCommand( } } } - rowsByLocation.remove(PluginSource.CLASSPATH); + rowsByLocation.remove(null); Set isolatedRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); long totalPlugins = isolatedRows.size(); long loadablePlugins = isolatedRows.stream().filter(Row::loadable).count(); From a18c83d2036ea8da26b0a78166a8b23456da4544 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 7 Aug 2023 14:24:54 -0700 Subject: [PATCH 19/28] fixup: add warning for incompatible classpath plugins not present on plugin path Signed-off-by: Greg Harris --- .../apache/kafka/tools/ConnectPluginPath.java | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 6a888c3dcd6c7..d85accaeea6d4 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -50,7 +50,9 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; +import java.util.EnumMap; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; @@ -61,6 +63,8 @@ import java.util.Objects; import java.util.Properties; import java.util.Set; +import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -291,6 +295,10 @@ private boolean compatible() { return loadable && hasManifest; } + private boolean incompatible() { + return !compatible(); + } + private String locationString() { return pluginLocation == null ? "classpath" : pluginLocation.toString(); } @@ -382,6 +390,15 @@ private static void endCommand( } } } + Set classpathOnlyPlugins = findClasspathOnlyPlugins(allRows); + Set classNames = classpathOnlyPlugins.stream().filter(Row::incompatible).map(row -> row.className).collect(Collectors.toSet()); + if (classNames.size() != 0) { + config.out.printf("%d plugins on the classpath are not compatible, and will not be auto-migrated. " + + "Please move these plugins to a plugin path location.%n", classNames.size()); + for (String className : classNames) { + config.out.printf("\t%s%n", className); + } + } rowsByLocation.remove(null); Set isolatedRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); long totalPlugins = isolatedRows.size(); @@ -520,4 +537,31 @@ private static Map> aliasCollisions(Set rows) { return aliasCollisions; } + /** + * Find plugins which are only present on the classpath. + *

This excludes plugins which are on the classpath, but which have a similar plugin appear on the plugin path. + * Plugins are considered similar if they have the same fully-qualified class name and plugin type. + * @param allRows All rows, including both classpath and plugin path plugins + * @return The set of rows from the classpath for which a similar plugin does not exist on the plugin path. + */ + private static Set findClasspathOnlyPlugins(Set allRows) { + Map> rowsByNameAndType = new LinkedHashMap<>(); + Function> emptyInnerMap = ignored -> new EnumMap<>(PluginType.class); + for (Row row : allRows) { + if (row.pluginLocation == null) { + rowsByNameAndType.computeIfAbsent(row.className, emptyInnerMap).put(row.type, row); + } + } + for (Row row : allRows) { + if (row.pluginLocation != null) { + rowsByNameAndType.computeIfAbsent(row.className, emptyInnerMap).remove(row.type); + } + } + return rowsByNameAndType.values() + .stream() + .map(EnumMap::values) + .flatMap(Collection::stream) + .collect(Collectors.toSet()); + } + } From ad8f26ad60adb4c61a1376b982844a2619681cea Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 7 Aug 2023 14:28:42 -0700 Subject: [PATCH 20/28] fixup: inject classpath string into PluginSource::location calls used for logging Signed-off-by: Greg Harris --- .../apache/kafka/connect/runtime/isolation/PluginSource.java | 4 ++-- .../main/java/org/apache/kafka/tools/ConnectPluginPath.java | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java index 665785a1c536e..d9b967c16ce33 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java @@ -34,8 +34,8 @@ public PluginSource(Path location, ClassLoader loader, URL[] urls) { this.urls = urls; } - public Path location() { - return location; + public String location() { + return location == null ? "classpath" : location.toString(); } public ClassLoader loader() { diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index d85accaeea6d4..355e49cd82404 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -240,8 +240,8 @@ public static void runCommand(Config config) throws TerseException { Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); PluginScanResult classpathPlugins = discoverPlugins(classpathSource, reflectionScanner, serviceLoaderScanner); Map> rowsByLocation = new LinkedHashMap<>(); - Set classpathRows = enumerateRows(classpathSource.location(), classpathManifests, classpathPlugins); - rowsByLocation.put(classpathSource.location(), classpathRows); + Set classpathRows = enumerateRows(null, classpathManifests, classpathPlugins); + rowsByLocation.put(null, classpathRows); ClassLoaderFactory factory = new ClassLoaderFactory(); try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { From d89b4e6517c09a4825cbdd07f8a5d4a6f1cea889 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Mon, 7 Aug 2023 14:52:26 -0700 Subject: [PATCH 21/28] fixup: replace path uniqueness soln with counting soln Signed-off-by: Greg Harris --- .../apache/kafka/tools/ConnectPluginPath.java | 43 ++++++++----------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 355e49cd82404..f590652451644 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -192,20 +192,6 @@ private static Set parseLocations(ArgumentParser parser, Namespace namespa return pluginLocations; } - private static Path makeDistinct(Path path) { - // When the same jar is present on the classpath and inside a pluginLocation, we still need to be able - // to distinguish between them. In order to make them more likely to be distinct, change the path to - // include an arbitrary change that makes them still resolve the same file, but fail direct equality checks. - Path parent = path.getParent(); - if (parent == null) { - // At the root of the filesystem, add the current directory marker to the end. - return path.resolve("."); - } else { - // If we have a parent directory, add the current directory marker there. - return parent.resolve(".").resolve(path.getFileName()); - } - } - enum Command { LIST } @@ -237,7 +223,7 @@ public static void runCommand(Config config) throws TerseException { ReflectionScanner reflectionScanner = new ReflectionScanner(); // Process the contents of the classpath to exclude it from later results. PluginSource classpathSource = PluginUtils.classpathPluginSource(parent); - Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); + Map> classpathManifests = findManifests(classpathSource, Collections.emptyMap()); PluginScanResult classpathPlugins = discoverPlugins(classpathSource, reflectionScanner, serviceLoaderScanner); Map> rowsByLocation = new LinkedHashMap<>(); Set classpathRows = enumerateRows(null, classpathManifests, classpathPlugins); @@ -247,8 +233,8 @@ public static void runCommand(Config config) throws TerseException { try (DelegatingClassLoader delegatingClassLoader = factory.newDelegatingClassLoader(parent)) { beginCommand(config); for (Path pluginLocation : config.locations) { - PluginSource source = PluginUtils.isolatedPluginSource(makeDistinct(pluginLocation), delegatingClassLoader, factory); - Map> manifests = findManifests(source, classpathManifests); + PluginSource source = PluginUtils.isolatedPluginSource(pluginLocation, delegatingClassLoader, factory); + Map> manifests = findManifests(source, classpathManifests); PluginScanResult plugins = discoverPlugins(source, reflectionScanner, serviceLoaderScanner); Set rows = enumerateRows(pluginLocation, manifests, plugins); rowsByLocation.put(pluginLocation, rows); @@ -317,7 +303,7 @@ public int hashCode() { } } - private static Set enumerateRows(Path pluginLocation, Map> manifests, PluginScanResult scanResult) { + private static Set enumerateRows(Path pluginLocation, Map> manifests, PluginScanResult scanResult) { Set rows = new HashSet<>(); // Perform a deep copy of the manifests because we're going to be mutating our copy. Map> unloadablePlugins = manifests.entrySet().stream() @@ -336,7 +322,7 @@ private static Set enumerateRows(Path pluginLocation, Map> manifests) { + private static Row newRow(Path pluginLocation, String className, PluginType type, String version, boolean loadable, Map> manifests) { Set rowAliases = new LinkedHashSet<>(); rowAliases.add(PluginUtils.simpleName(className)); rowAliases.add(PluginUtils.prunedName(className, type)); @@ -457,8 +443,8 @@ public int hashCode() { } } - private static Map> findManifests(PluginSource source, Map> exclude) { - Map> manifests = new HashMap<>(); + private static Map> findManifests(PluginSource source, Map> exclude) { + Map> manifests = new LinkedHashMap<>(); for (PluginType type : PluginType.values()) { try { Enumeration resources = source.loader().getResources(MANIFEST_PREFIX + type.superClass().getName()); @@ -466,9 +452,7 @@ private static Map> findManifests(PluginSource source URL url = resources.nextElement(); for (String className : parse(url)) { ManifestEntry e = new ManifestEntry(url.toURI(), className, type); - if (!exclude.containsKey(className) || !exclude.get(className).contains(e)) { - manifests.computeIfAbsent(className, ignored -> new HashSet<>()).add(e); - } + manifests.computeIfAbsent(className, ignored -> new ArrayList<>()).add(e); } } } catch (URISyntaxException e) { @@ -477,6 +461,17 @@ private static Map> findManifests(PluginSource source throw new UncheckedIOException(e); } } + for (Map.Entry> entry : exclude.entrySet()) { + String className = entry.getKey(); + List excluded = entry.getValue(); + // Note this must be a remove and not removeAll, because we want to remove only one copy at a time. + // If the same jar is present on the classpath and plugin path, then manifests will contain 2 identical + // ManifestEntry instances, with a third copy in the excludes. After the excludes are processed, + // manifests should contain exactly one copy of the ManifestEntry. + for (ManifestEntry e : excluded) { + manifests.getOrDefault(className, Collections.emptyList()).remove(e); + } + } return manifests; } From c9b855b079555e28b754e3fa5b7b82af760c45f7 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Tue, 8 Aug 2023 11:49:53 -0700 Subject: [PATCH 22/28] fixup: Add assertions on list table output Signed-off-by: Greg Harris --- .../runtime/isolation/TestPlugins.java | 2 +- .../apache/kafka/tools/ConnectPluginPath.java | 7 +- .../kafka/tools/ConnectPluginPathTest.java | 340 ++++++++++++------ 3 files changed, 245 insertions(+), 104 deletions(-) diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java index 8f675a5a42e78..347d4140590b2 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java @@ -125,7 +125,7 @@ public enum TestPlugin { /** * A plugin which is incorrectly packaged, and is missing a superclass definition. */ - BAD_PACKAGING_MISSING_SUPERCLASS("bad-packaging", "test.plugins.MissingSuperclass", false, REMOVE_CLASS_FILTER), + BAD_PACKAGING_MISSING_SUPERCLASS("bad-packaging", "test.plugins.MissingSuperclassConverter", false, REMOVE_CLASS_FILTER), /** * A plugin which is packaged with other incorrectly packaged plugins, but itself has no issues loading. */ diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index f8a54354a9b68..7ccc76471cea7 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -70,7 +70,7 @@ public class ConnectPluginPath { private static final String MANIFEST_PREFIX = "META-INF/services/"; - private static final Object[] LIST_TABLE_COLUMNS = { + public static final Object[] LIST_TABLE_COLUMNS = { "pluginName", "firstAlias", "secondAlias", @@ -80,6 +80,7 @@ public class ConnectPluginPath { "hasManifest", "pluginLocation" // last because it is least important and most repetitive }; + public static final String NO_ALIAS = "N/A"; public static void main(String[] args) { Exit.exit(mainNoExit(args, System.out, System.err)); @@ -340,8 +341,8 @@ private static void beginCommand(Config config) { private static void handlePlugin(Config config, Row row) { if (config.command == Command.LIST) { - String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : "N/A"; - String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : "N/A"; + String firstAlias = row.aliases.size() > 0 ? row.aliases.get(0) : NO_ALIAS; + String secondAlias = row.aliases.size() > 1 ? row.aliases.get(1) : NO_ALIAS; listTablePrint(config, row.className, firstAlias, diff --git a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java index 7440de59f32f2..6581c300728a5 100644 --- a/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java +++ b/tools/src/test/java/org/apache/kafka/tools/ConnectPluginPathTest.java @@ -38,13 +38,17 @@ import java.io.PrintStream; import java.io.UncheckedIOException; import java.io.UnsupportedEncodingException; +import java.net.MalformedURLException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Properties; import java.util.Set; @@ -52,16 +56,227 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; public class ConnectPluginPathTest { private static final Logger log = LoggerFactory.getLogger(ConnectPluginPathTest.class); + private static final int NAME_COL = 0; + private static final int ALIAS1_COL = 1; + private static final int ALIAS2_COL = 2; + private static final int VERSION_COL = 3; + private static final int TYPE_COL = 4; + private static final int LOADABLE_COL = 5; + private static final int MANIFEST_COL = 6; + private static final int LOCATION_COL = 7; + @TempDir public Path workspace; + @BeforeAll + public static void setUp() { + // Work around a circular-dependency in TestPlugins. + TestPlugins.pluginPath(); + } + + + @Test + public void testNoArguments() { + CommandResult res = runCommand(); + assertNotEquals(0, res.returnCode); + } + + @Test + public void testListNoArguments() { + CommandResult res = runCommand( + "list" + ); + assertNotEquals(0, res.returnCode); + } + + @ParameterizedTest + @EnumSource + public void testListOneLocation(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-location", + setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN) + ); + Map> table = assertListSuccess(res); + assertNonMigratedPluginsPresent(table); + } + + @ParameterizedTest + @EnumSource + public void testListMultipleLocations(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-location", + setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN), + "--plugin-location", + setupLocation(workspace.resolve("location-b"), type, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) + ); + Map> table = assertListSuccess(res); + assertNonMigratedPluginsPresent(table); + assertPluginsAreCompatible(table, + TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE); + } + + @ParameterizedTest + @EnumSource + public void testListOnePluginPath(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-path", + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) + ); + Map> table = assertListSuccess(res); + assertPluginsAreCompatible(table, + TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE); + } + + @ParameterizedTest + @EnumSource + public void testListMultiplePluginPaths(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--plugin-path", + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE), + "--plugin-path", + setupPluginPathElement(workspace.resolve("path-b"), type, + TestPlugins.TestPlugin.SAMPLING_HEADER_CONVERTER, TestPlugins.TestPlugin.ALIASED_STATIC_FIELD) + ); + Map> table = assertListSuccess(res); + assertPluginsAreCompatible(table, + TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE, + TestPlugins.TestPlugin.ALIASED_STATIC_FIELD); + } + + @ParameterizedTest + @EnumSource + public void testListOneWorkerConfig(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--worker-config", + setupWorkerConfig(workspace.resolve("worker.properties"), + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.BAD_PACKAGING_CO_LOCATED)) + ); + Map> table = assertListSuccess(res); + assertBadPackagingPluginsPresent(table); + } + + @ParameterizedTest + @EnumSource + public void testListMultipleWorkerConfigs(PluginLocationType type) { + CommandResult res = runCommand( + "list", + "--worker-config", + setupWorkerConfig(workspace.resolve("worker-a.properties"), + setupPluginPathElement(workspace.resolve("path-a"), type, + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN)), + "--worker-config", + setupWorkerConfig(workspace.resolve("worker-b.properties"), + setupPluginPathElement(workspace.resolve("path-b"), type, + TestPlugins.TestPlugin.SERVICE_LOADER)) + ); + Map> table = assertListSuccess(res); + assertNonMigratedPluginsPresent(table); + assertPluginsAreCompatible(table, + TestPlugins.TestPlugin.SERVICE_LOADER); + } + + + private static Map> assertListSuccess(CommandResult result) { + assertEquals(0, result.returnCode); + Map> table = parseTable(result.out); + assertIsolatedPluginsInOutput(result.reflective, table); + return table; + } + + private static void assertPluginsAreCompatible(Map> table, TestPlugins.TestPlugin... plugins) { + assertPluginMigrationStatus(table, true, true, plugins); + } + + private static void assertNonMigratedPluginsPresent(Map> table) { + assertPluginMigrationStatus(table, true, false, + TestPlugins.TestPlugin.NON_MIGRATED_CONVERTER, + TestPlugins.TestPlugin.NON_MIGRATED_HEADER_CONVERTER, + TestPlugins.TestPlugin.NON_MIGRATED_PREDICATE, + TestPlugins.TestPlugin.NON_MIGRATED_SINK_CONNECTOR, + TestPlugins.TestPlugin.NON_MIGRATED_SOURCE_CONNECTOR, + TestPlugins.TestPlugin.NON_MIGRATED_TRANSFORMATION); + // This plugin is partially compatible + assertPluginMigrationStatus(table, true, null, + TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN); + } + + private static void assertBadPackagingPluginsPresent(Map> table) { + assertPluginsAreCompatible(table, + TestPlugins.TestPlugin.BAD_PACKAGING_CO_LOCATED, + TestPlugins.TestPlugin.BAD_PACKAGING_VERSION_METHOD_THROWS_CONNECTOR); + assertPluginMigrationStatus(table, false, true, + TestPlugins.TestPlugin.BAD_PACKAGING_MISSING_SUPERCLASS, + TestPlugins.TestPlugin.BAD_PACKAGING_STATIC_INITIALIZER_THROWS_CONNECTOR, + TestPlugins.TestPlugin.BAD_PACKAGING_DEFAULT_CONSTRUCTOR_THROWS_CONNECTOR, + TestPlugins.TestPlugin.BAD_PACKAGING_DEFAULT_CONSTRUCTOR_PRIVATE_CONNECTOR, + TestPlugins.TestPlugin.BAD_PACKAGING_NO_DEFAULT_CONSTRUCTOR_CONNECTOR, + TestPlugins.TestPlugin.BAD_PACKAGING_NO_DEFAULT_CONSTRUCTOR_CONVERTER, + TestPlugins.TestPlugin.BAD_PACKAGING_NO_DEFAULT_CONSTRUCTOR_OVERRIDE_POLICY, + TestPlugins.TestPlugin.BAD_PACKAGING_INNER_CLASS_CONNECTOR, + TestPlugins.TestPlugin.BAD_PACKAGING_STATIC_INITIALIZER_THROWS_REST_EXTENSION); + } + + + private static void assertIsolatedPluginsInOutput(PluginScanResult reflectiveResult, Map> table) { + reflectiveResult.forEach(pluginDesc -> { + if (pluginDesc.location().equals("classpath")) { + // Classpath plugins do not appear in list output + return; + } + assertTrue(table.containsKey(pluginDesc.className()), "Plugin " + pluginDesc.className() + " does not appear in list output"); + boolean foundType = false; + for (String[] row : table.get(pluginDesc.className())) { + if (row[TYPE_COL].equals(pluginDesc.typeName())) { + foundType = true; + assertTrue(row[ALIAS1_COL].equals(ConnectPluginPath.NO_ALIAS) || row[ALIAS1_COL].equals(PluginUtils.simpleName(pluginDesc))); + assertTrue(row[ALIAS2_COL].equals(ConnectPluginPath.NO_ALIAS) || row[ALIAS2_COL].equals(PluginUtils.prunedName(pluginDesc))); + assertEquals(pluginDesc.version(), row[VERSION_COL]); + try { + Path pluginLocation = Paths.get(row[LOCATION_COL]); + // This transforms the raw path `/path/to/somewhere` to the url `file:/path/to/somewhere` + String pluginLocationUrl = pluginLocation.toUri().toURL().toString(); + assertEquals(pluginDesc.location(), pluginLocationUrl); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + } + } + assertTrue(foundType, "Plugin " + pluginDesc.className() + " does not have row for " + pluginDesc.typeName()); + }); + } + + private static void assertPluginMigrationStatus(Map> table, Boolean loadable, Boolean compatible, TestPlugins.TestPlugin... plugins) { + for (TestPlugins.TestPlugin plugin : plugins) { + assertTrue(table.containsKey(plugin.className()), "Plugin " + plugin.className() + " does not appear in list output"); + for (String[] row : table.get(plugin.className())) { + log.info("row" + Arrays.toString(row)); + if (loadable != null) { + assertEquals(loadable, Boolean.parseBoolean(row[LOADABLE_COL]), "Plugin loadable column for " + plugin.className() + " incorrect"); + } + if (compatible != null) { + assertEquals(compatible, Boolean.parseBoolean(row[MANIFEST_COL]), "Plugin hasManifest column for " + plugin.className() + " incorrect"); + } + } + } + } + private enum PluginLocationType { CLASS_HIERARCHY, SINGLE_JAR, @@ -81,12 +296,6 @@ public String toString() { } } - @BeforeAll - public static void setUp() { - // Work around a circular-dependency in TestPlugins. - TestPlugins.pluginPath(); - } - /** * Populate a writable disk path to be usable as a single plugin location. * The returned path will be usable as a single path. @@ -185,7 +394,7 @@ public String toString() { * @param pluginPathElements * @return */ - private WorkerConfig setupWorkerConfig(Path path, PluginPathElement... pluginPathElements) { + private static WorkerConfig setupWorkerConfig(Path path, PluginPathElement... pluginPathElements) { path.getParent().toFile().mkdirs(); Properties properties = new Properties(); String pluginPath = Arrays.stream(pluginPathElements) @@ -216,7 +425,7 @@ public CommandResult(int returnCode, String out, String err, PluginScanResult re PluginScanResult serviceLoading; } - private CommandResult runCommand(Object... args) { + private static CommandResult runCommand(Object... args) { ByteArrayOutputStream out = new ByteArrayOutputStream(); ByteArrayOutputStream err = new ByteArrayOutputStream(); try { @@ -279,98 +488,29 @@ private static Set getPluginLocations(Object[] args) { .collect(Collectors.toSet()); } - @Test - public void testNoArguments() { - CommandResult res = runCommand(); - assertNotEquals(0, res.returnCode); - } - - @Test - public void testListNoArguments() { - CommandResult res = runCommand( - "list" - ); - assertNotEquals(0, res.returnCode); - } - - @ParameterizedTest - @EnumSource - public void testListOneLocation(PluginLocationType type) { - CommandResult res = runCommand( - "list", - "--plugin-location", - setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN) - ); - assertEquals(0, res.returnCode); - } - - @ParameterizedTest - @EnumSource - public void testListMultipleLocations(PluginLocationType type) { - CommandResult res = runCommand( - "list", - "--plugin-location", - setupLocation(workspace.resolve("location-a"), type, TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN), - "--plugin-location", - setupLocation(workspace.resolve("location-b"), type, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) - ); - assertEquals(0, res.returnCode); - } - - @ParameterizedTest - @EnumSource - public void testListOnePluginPath(PluginLocationType type) { - CommandResult res = runCommand( - "list", - "--plugin-path", - setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE) - ); - assertEquals(0, res.returnCode); - } - @ParameterizedTest - @EnumSource - public void testListMultiplePluginPaths(PluginLocationType type) { - CommandResult res = runCommand( - "list", - "--plugin-path", - setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN, TestPlugins.TestPlugin.SAMPLING_CONFIGURABLE), - "--plugin-path", - setupPluginPathElement(workspace.resolve("path-b"), type, - TestPlugins.TestPlugin.SAMPLING_HEADER_CONVERTER, TestPlugins.TestPlugin.ALIASED_STATIC_FIELD) - ); - assertEquals(0, res.returnCode); - } - - @ParameterizedTest - @EnumSource - public void testListOneWorkerConfig(PluginLocationType type) { - CommandResult res = runCommand( - "list", - "--worker-config", - setupWorkerConfig(workspace.resolve("worker.properties"), - setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN)) - ); - assertEquals(0, res.returnCode); - } - - @ParameterizedTest - @EnumSource - public void testListMultipleWorkerConfigs(PluginLocationType type) { - CommandResult res = runCommand( - "list", - "--worker-config", - setupWorkerConfig(workspace.resolve("worker-a.properties"), - setupPluginPathElement(workspace.resolve("path-a"), type, - TestPlugins.TestPlugin.NON_MIGRATED_MULTI_PLUGIN)), - "--worker-config", - setupWorkerConfig(workspace.resolve("worker-b.properties"), - setupPluginPathElement(workspace.resolve("path-b"), type, - TestPlugins.TestPlugin.SERVICE_LOADER)) - ); - assertEquals(0, res.returnCode); + /** + * Parse the main table of the list command. + *

Map is keyed on the plugin name, with a list of rows which referred to that name if there are multiple. + * Each row is pre-split into columns. + * @param listOutput An executed list command + * @return A parsed form of the table grouped by plugin class names + */ + private static Map> parseTable(String listOutput) { + // Split on the empty line which should appear in the output. + String[] sections = listOutput.split("\n\\s*\n"); + assertTrue(sections.length > 1, "No empty line in list output"); + String[] rows = sections[0].split("\n"); + Map> table = new HashMap<>(); + // Assert that the first row is the header + assertArrayEquals(ConnectPluginPath.LIST_TABLE_COLUMNS, rows[0].split("\t"), "Table header doesn't have the right columns"); + // Skip the header to parse the rows in the table. + for (int i = 1; i < rows.length; i++) { + // group rows by + String[] row = rows[i].split("\t"); + assertEquals(ConnectPluginPath.LIST_TABLE_COLUMNS.length, row.length, "Table row is the wrong length"); + table.computeIfAbsent(row[NAME_COL], ignored -> new ArrayList<>()).add(row); + } + return table; } } From 9742d90f0c2ecbd9899dcea3e221b1c504afe1de Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Tue, 8 Aug 2023 15:17:46 -0700 Subject: [PATCH 23/28] fixup: add javadocs for new test-plugins, add more interfaces to the multi plugin Signed-off-by: Greg Harris --- .../runtime/isolation/TestPlugins.java | 21 +++++++++++++++++++ ...olicy.ConnectorClientConfigOverridePolicy} | 0 .../test/plugins/NonMigratedMultiPlugin.java | 11 +++++++++- 3 files changed, 31 insertions(+), 1 deletion(-) rename connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/{org.apache.kafka.connect.storage.Converter => org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy} (100%) diff --git a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java index 347d4140590b2..d6fd3cd54bca2 100644 --- a/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java +++ b/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/isolation/TestPlugins.java @@ -174,12 +174,33 @@ public enum TestPlugin { * A ServiceLoader discovered plugin which subclasses another plugin which is present on the classpath */ SUBCLASS_OF_CLASSPATH_OVERRIDE_POLICY("subclass-of-classpath", "test.plugins.SubclassOfClasspathOverridePolicy"), + /** + * A converter which does not have a corresponding ServiceLoader manifest + */ NON_MIGRATED_CONVERTER("non-migrated", "test.plugins.NonMigratedConverter", false), + /** + * A header converter which does not have a corresponding ServiceLoader manifest + */ NON_MIGRATED_HEADER_CONVERTER("non-migrated", "test.plugins.NonMigratedHeaderConverter", false), + /** + * A plugin which implements multiple interfaces, and has ServiceLoader manifests for some interfaces and not others. + */ NON_MIGRATED_MULTI_PLUGIN("non-migrated", "test.plugins.NonMigratedMultiPlugin", false), + /** + * A predicate which does not have a corresponding ServiceLoader manifest + */ NON_MIGRATED_PREDICATE("non-migrated", "test.plugins.NonMigratedPredicate", false), + /** + * A sink connector which does not have a corresponding ServiceLoader manifest + */ NON_MIGRATED_SINK_CONNECTOR("non-migrated", "test.plugins.NonMigratedSinkConnector", false), + /** + * A source connector which does not have a corresponding ServiceLoader manifest + */ NON_MIGRATED_SOURCE_CONNECTOR("non-migrated", "test.plugins.NonMigratedSourceConnector", false), + /** + * A transformation which does not have a corresponding ServiceLoader manifest + */ NON_MIGRATED_TRANSFORMATION("non-migrated", "test.plugins.NonMigratedTransformation", false); private final String resourceDir; diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.storage.Converter b/connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy similarity index 100% rename from connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.storage.Converter rename to connect/runtime/src/test/resources/test-plugins/non-migrated/META-INF/services/org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy diff --git a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java index 6d335cf6481dc..a82b82f297439 100644 --- a/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java +++ b/connect/runtime/src/test/resources/test-plugins/non-migrated/test/plugins/NonMigratedMultiPlugin.java @@ -18,7 +18,10 @@ package test.plugins; import org.apache.kafka.common.config.ConfigDef; +import org.apache.kafka.common.config.ConfigValue; import org.apache.kafka.connect.connector.ConnectRecord; +import org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy; +import org.apache.kafka.connect.connector.policy.ConnectorClientConfigRequest; import org.apache.kafka.connect.data.Schema; import org.apache.kafka.connect.data.SchemaAndValue; import org.apache.kafka.connect.storage.Converter; @@ -26,6 +29,7 @@ import org.apache.kafka.connect.transforms.predicates.Predicate; import org.apache.kafka.connect.transforms.Transformation; +import java.util.List; import java.util.Map; /** @@ -33,7 +37,7 @@ * See {@link org.apache.kafka.connect.runtime.isolation.TestPlugins}. *

Class which is not migrated to include a service loader manifest. */ -public final class NonMigratedMultiPlugin implements Converter, HeaderConverter, Predicate, Transformation { +public final class NonMigratedMultiPlugin implements Converter, HeaderConverter, Predicate, Transformation, ConnectorClientConfigOverridePolicy { @Override public void configure(Map configs, boolean isKey) { @@ -84,4 +88,9 @@ public void close() { public void configure(Map configs) { } + + @Override + public List validate(ConnectorClientConfigRequest connectorClientConfigRequest) { + return null; + } } From 1955722a43574b2d4af4e46f7a874dd016ce0f6f Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 9 Aug 2023 09:30:26 -0700 Subject: [PATCH 24/28] fixup: move logging source.location() functionality to source.toString() Signed-off-by: Greg Harris --- .../kafka/connect/runtime/isolation/PluginScanner.java | 10 +++++----- .../kafka/connect/runtime/isolation/PluginSource.java | 8 ++++++-- .../connect/runtime/isolation/ReflectionScanner.java | 8 ++++---- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginScanner.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginScanner.java index acb5b668cf301..4e99d3de53df8 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginScanner.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginScanner.java @@ -72,7 +72,7 @@ public PluginScanResult discoverPlugins(Set sources) { } private PluginScanResult scanUrlsAndAddPlugins(PluginSource source) { - log.info("Loading plugin from: {}", source.location()); + log.info("Loading plugin from: {}", source); if (log.isDebugEnabled()) { log.debug("Loading plugin urls: {}", Arrays.toString(source.urls())); } @@ -136,13 +136,13 @@ protected SortedSet> getServiceLoaderPluginDesc(PluginType typ pluginImpl = handleLinkageError(type, source, iterator::next); } catch (ServiceConfigurationError t) { log.error("Failed to discover {} in {}{}", - type.simpleName(), source.location(), reflectiveErrorDescription(t.getCause()), t); + type.simpleName(), source, reflectiveErrorDescription(t.getCause()), t); continue; } Class pluginKlass = (Class) pluginImpl.getClass(); if (pluginKlass.getClassLoader() != source.loader()) { log.debug("{} from other classloader {} is visible from {}, excluding to prevent isolated loading", - type.simpleName(), pluginKlass.getClassLoader(), source.location()); + type.simpleName(), pluginKlass.getClassLoader(), source); continue; } result.add(pluginDesc(pluginKlass, versionFor(pluginImpl), type, source)); @@ -181,14 +181,14 @@ private U handleLinkageError(PluginType type, PluginSource source, Supplier< || !Objects.equals(lastError.getClass(), t.getClass()) || !Objects.equals(lastError.getMessage(), t.getMessage())) { log.error("Failed to discover {} in {}{}", - type.simpleName(), source.location(), reflectiveErrorDescription(t.getCause()), t); + type.simpleName(), source, reflectiveErrorDescription(t.getCause()), t); } lastError = t; } } log.error("Received excessive ServiceLoader errors: assuming the runtime ServiceLoader implementation cannot " + "skip faulty implementations. Use a different JRE, or resolve LinkageErrors for plugins in {}", - source.location(), lastError); + source, lastError); throw lastError; } diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java index d9b967c16ce33..3a10e2ba43f98 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java @@ -34,8 +34,8 @@ public PluginSource(Path location, ClassLoader loader, URL[] urls) { this.urls = urls; } - public String location() { - return location == null ? "classpath" : location.toString(); + public Path location() { + return location; } public ClassLoader loader() { @@ -64,4 +64,8 @@ public int hashCode() { result = 31 * result + Arrays.hashCode(urls); return result; } + + public String toString() { + return location == null ? "classpath" : location.toString(); + } } diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java index ad5c00c42ec6f..332f8ea9091a5 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/ReflectionScanner.java @@ -117,26 +117,26 @@ private SortedSet> getPluginDesc( plugins = reflections.getSubTypesOf((Class) type.superClass()); } catch (ReflectionsException e) { log.debug("Reflections scanner could not find any {} in {} for URLs: {}", - type, source.location(), source.urls(), e); + type, source, source.urls(), e); return Collections.emptySortedSet(); } SortedSet> result = new TreeSet<>(); for (Class pluginKlass : plugins) { if (!PluginUtils.isConcrete(pluginKlass)) { - log.debug("Skipping {} in {} as it is not concrete implementation", pluginKlass, source.location()); + log.debug("Skipping {} in {} as it is not concrete implementation", pluginKlass, source); continue; } if (pluginKlass.getClassLoader() != source.loader()) { log.debug("{} from other classloader {} is visible from {}, excluding to prevent isolated loading", - pluginKlass, pluginKlass.getClassLoader(), source.location()); + pluginKlass, pluginKlass.getClassLoader(), source); continue; } try (LoaderSwap loaderSwap = withClassLoader(source.loader())) { result.add(pluginDesc(pluginKlass, versionFor(pluginKlass), type, source)); } catch (ReflectiveOperationException | LinkageError e) { log.error("Failed to discover {} in {}: Unable to instantiate {}{}", - type.simpleName(), source.location(), pluginKlass.getSimpleName(), + type.simpleName(), source, pluginKlass.getSimpleName(), reflectiveErrorDescription(e), e); } } From f858b9a444d2f24aa313decd08fa49a83bdf9ce0 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 9 Aug 2023 09:40:23 -0700 Subject: [PATCH 25/28] fixup: incorrect comment Signed-off-by: Greg Harris --- .../src/main/java/org/apache/kafka/tools/ConnectPluginPath.java | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index 7ccc76471cea7..c91c7527d30de 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -476,7 +476,6 @@ private static Map> findManifests(PluginSource sourc } // Based on implementation from ServiceLoader.LazyClassPathLookupIterator from OpenJDK11 - // visible for testing private static Set parse(URL u) { Set names = new LinkedHashSet<>(); // preserve insertion order try { From b05dae61419c0992d5d6e952dcd37931607f8054 Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Wed, 9 Aug 2023 09:43:30 -0700 Subject: [PATCH 26/28] fixup: checkstyle Signed-off-by: Greg Harris --- .../org/apache/kafka/connect/runtime/isolation/PluginSource.java | 1 - 1 file changed, 1 deletion(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java index 3a10e2ba43f98..6cc19343b5e77 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginSource.java @@ -18,7 +18,6 @@ import java.net.URL; import java.nio.file.Path; -import java.nio.file.Paths; import java.util.Arrays; import java.util.Objects; From 9c1dc0e6b213482fbe7b6d5520256dbd78f2604b Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 10 Aug 2023 10:30:21 -0700 Subject: [PATCH 27/28] fixup: revert changes to alias computation, only provide aliases for loadable plugins Signed-off-by: Greg Harris --- .../runtime/isolation/PluginUtils.java | 19 ++++----------- .../apache/kafka/tools/ConnectPluginPath.java | 24 +++++++++---------- 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java index 4c340f5dfdfbd..d9036c03a4f76 100644 --- a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java +++ b/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java @@ -385,11 +385,6 @@ public static String simpleName(PluginDesc plugin) { return plugin.pluginClass().getSimpleName(); } - public static String simpleName(String fullClassName) { - return fullClassName.substring(fullClassName.lastIndexOf('.') + 1); - } - - /** * Remove the plugin type name at the end of a plugin class name, if such suffix is present. * This method is meant to be used to extract plugin aliases. @@ -398,22 +393,18 @@ public static String simpleName(String fullClassName) { * @return the pruned simple class name of the plugin. */ public static String prunedName(PluginDesc plugin) { - return prunedName(plugin.className(), plugin.type()); - } - - public static String prunedName(String fullClassName, PluginType type) { // It's currently simpler to switch on type than do pattern matching. - switch (type) { + switch (plugin.type()) { case SOURCE: case SINK: - return prunePluginName(fullClassName, "Connector"); + return prunePluginName(plugin, "Connector"); default: - return prunePluginName(fullClassName, type.simpleName()); + return prunePluginName(plugin, plugin.type().simpleName()); } } - private static String prunePluginName(String fullClassName, String suffix) { - String simple = simpleName(fullClassName); + private static String prunePluginName(PluginDesc plugin, String suffix) { + String simple = plugin.pluginClass().getSimpleName(); int pos = simple.lastIndexOf(suffix); if (pos > 0) { return simple.substring(0, pos); diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index c91c7527d30de..bfd4b258ea9ac 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -310,24 +310,24 @@ private static Set enumerateRows(Path pluginLocation, Map new HashSet<>(e.getValue()))); scanResult.forEach(pluginDesc -> { // Emit a loadable row for this scan result, since it was found during plugin discovery - rows.add(newRow(pluginLocation, pluginDesc.className(), pluginDesc.type(), pluginDesc.version(), true, manifests)); + Set rowAliases = new LinkedHashSet<>(); + rowAliases.add(PluginUtils.simpleName(pluginDesc)); + rowAliases.add(PluginUtils.prunedName(pluginDesc)); + rows.add(newRow(pluginLocation, pluginDesc.className(), new ArrayList<>(rowAliases), pluginDesc.type(), pluginDesc.version(), true, manifests)); // Remove the ManifestEntry if it has the same className and type as one of the loadable plugins. unloadablePlugins.getOrDefault(pluginDesc.className(), Collections.emptySet()).removeIf(entry -> entry.type == pluginDesc.type()); }); unloadablePlugins.values().forEach(entries -> entries.forEach(entry -> { // Emit a non-loadable row, since all the loadable rows showed up in the previous iteration. // Two ManifestEntries may produce the same row if they have different URIs - rows.add(newRow(pluginLocation, entry.className, entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests)); + rows.add(newRow(pluginLocation, entry.className, Collections.emptyList(), entry.type, PluginDesc.UNDEFINED_VERSION, false, manifests)); })); return rows; } - private static Row newRow(Path pluginLocation, String className, PluginType type, String version, boolean loadable, Map> manifests) { - Set rowAliases = new LinkedHashSet<>(); - rowAliases.add(PluginUtils.simpleName(className)); - rowAliases.add(PluginUtils.prunedName(className, type)); + private static Row newRow(Path pluginLocation, String className, List rowAliases, PluginType type, String version, boolean loadable, Map> manifests) { boolean hasManifest = manifests.containsKey(className) && manifests.get(className).stream().anyMatch(e -> e.type == type); - return new Row(pluginLocation, className, type, version, new ArrayList<>(rowAliases), loadable, hasManifest); + return new Row(pluginLocation, className, type, version, rowAliases, loadable, hasManifest); } private static void beginCommand(Config config) { @@ -523,12 +523,10 @@ private static int parseLine(URL u, BufferedReader r, int lc, Set names) } private static Map> aliasCollisions(Set rows) { - Map> aliasCollisions = new HashMap<>(); - rows.forEach(row -> { - aliasCollisions.computeIfAbsent(PluginUtils.simpleName(row.className), ignored -> new HashSet<>()).add(row.className); - aliasCollisions.computeIfAbsent(PluginUtils.prunedName(row.className, row.type), ignored -> new HashSet<>()).add(row.className); - }); - return aliasCollisions; + Map> collisions = new HashMap<>(); + rows.forEach(row -> row.aliases.forEach(alias -> + collisions.computeIfAbsent(alias, ignored -> new HashSet<>()).add(row.className))); + return collisions; } /** From 0d7077ac2b9f3f5a40cf45c08e4cc3c8106536db Mon Sep 17 00:00:00 2001 From: Greg Harris Date: Thu, 10 Aug 2023 13:05:39 -0700 Subject: [PATCH 28/28] fixup: remove unnecessary functionality and warnings Signed-off-by: Greg Harris --- .../apache/kafka/tools/ConnectPluginPath.java | 73 +------------------ 1 file changed, 3 insertions(+), 70 deletions(-) diff --git a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java index bfd4b258ea9ac..c1b0f55259fdc 100644 --- a/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java +++ b/tools/src/main/java/org/apache/kafka/tools/ConnectPluginPath.java @@ -50,11 +50,8 @@ import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; -import java.util.EnumMap; import java.util.Enumeration; -import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -63,7 +60,6 @@ import java.util.Objects; import java.util.Properties; import java.util.Set; -import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -364,42 +360,14 @@ private static void endCommand( if (config.command == Command.LIST) { // end the table with an empty line to enable users to separate the table from the summary. config.out.println(); - Set allRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); - Map> aliasCollisions = aliasCollisions(allRows); - for (Map.Entry> entry : aliasCollisions.entrySet()) { - String alias = entry.getKey(); - Set classNames = entry.getValue(); - if (classNames.size() != 1) { - config.out.printf("%s is an ambiguous alias and will not be usable. One of the fully qualified classes must be used instead:%n", alias); - for (String className : classNames) { - config.out.printf("\t%s%n", className); - } - } - } - Set classpathOnlyPlugins = findClasspathOnlyPlugins(allRows); - Set classNames = classpathOnlyPlugins.stream().filter(Row::incompatible).map(row -> row.className).collect(Collectors.toSet()); - if (classNames.size() != 0) { - config.out.printf("%d plugins on the classpath are not compatible, and will not be auto-migrated. " + - "Please move these plugins to a plugin path location.%n", classNames.size()); - for (String className : classNames) { - config.out.printf("\t%s%n", className); - } - } rowsByLocation.remove(null); Set isolatedRows = rowsByLocation.values().stream().flatMap(Set::stream).collect(Collectors.toSet()); long totalPlugins = isolatedRows.size(); long loadablePlugins = isolatedRows.stream().filter(Row::loadable).count(); long compatiblePlugins = isolatedRows.stream().filter(Row::compatible).count(); - long totalLocations = rowsByLocation.size(); - long compatibleLocations = rowsByLocation.values().stream().filter(location -> location.stream().allMatch(Row::compatible)).count(); - boolean allLocationsCompatible = isolatedRows.stream().allMatch(Row::compatible); - config.out.printf("Total plugins: \t%d%n", totalPlugins); - config.out.printf("Loadable plugins: \t%d%n", loadablePlugins); - config.out.printf("Compatible plugins: \t%d%n", compatiblePlugins); - config.out.printf("Total locations: \t%d%n", totalLocations); - config.out.printf("Compatible locations: \t%d%n", compatibleLocations); - config.out.printf("All locations compatible?\t%b%n", allLocationsCompatible); - + config.out.printf("Total plugins: \t%d%n", totalPlugins); + config.out.printf("Loadable plugins: \t%d%n", loadablePlugins); + config.out.printf("Compatible plugins: \t%d%n", compatiblePlugins); } } @@ -521,39 +489,4 @@ private static int parseLine(URL u, BufferedReader r, int lc, Set names) } return lc + 1; } - - private static Map> aliasCollisions(Set rows) { - Map> collisions = new HashMap<>(); - rows.forEach(row -> row.aliases.forEach(alias -> - collisions.computeIfAbsent(alias, ignored -> new HashSet<>()).add(row.className))); - return collisions; - } - - /** - * Find plugins which are only present on the classpath. - *

This excludes plugins which are on the classpath, but which have a similar plugin appear on the plugin path. - * Plugins are considered similar if they have the same fully-qualified class name and plugin type. - * @param allRows All rows, including both classpath and plugin path plugins - * @return The set of rows from the classpath for which a similar plugin does not exist on the plugin path. - */ - private static Set findClasspathOnlyPlugins(Set allRows) { - Map> rowsByNameAndType = new LinkedHashMap<>(); - Function> emptyInnerMap = ignored -> new EnumMap<>(PluginType.class); - for (Row row : allRows) { - if (row.pluginLocation == null) { - rowsByNameAndType.computeIfAbsent(row.className, emptyInnerMap).put(row.type, row); - } - } - for (Row row : allRows) { - if (row.pluginLocation != null) { - rowsByNameAndType.computeIfAbsent(row.className, emptyInnerMap).remove(row.type); - } - } - return rowsByNameAndType.values() - .stream() - .map(EnumMap::values) - .flatMap(Collection::stream) - .collect(Collectors.toSet()); - } - }