KAFKA-13510: Connect APIs to list all connector plugins and retrieve … by mimaison · Pull Request #11572 · apache/kafka

mimaison · 2021-12-07T10:42:27Z

…their configdefs

Implements KIP-769

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…their configdefs Implements KIP-769: https://cwiki.apache.org/confluence/display/KAFKA/KIP-769%3A+Connect+APIs+to+list+all+connector+plugins+and+retrieve+their+configuration+definitions

mimaison · 2022-02-23T11:30:06Z

@tombentley @vvcephei @C0urante As you voted on the KIP, can you take a look? Thanks

C0urante

Thanks @mimaison. I've taken a pass at all of the functional parts; leaving the testing logic for later. Feel free to ignore (and resolve) anything prefaced with "nit" without commenting if you don't want to address it for whatever reason.

mimaison · 2022-02-25T16:36:11Z

Thanks @C0urante for the review, all good suggestions. I believe I've addressed them all.

C0urante

Looking good! I've completed a pass of every file, including test cases. Once everything here is addressed (with code changes or discussion) I think this will be ready to go.

mimaison · 2022-02-28T18:21:11Z

Thanks @C0urante for the review! Again you made some very good suggestions. I've pushed an update.

C0urante

LGTM, thanks Mickael! Looking forward to seeing this land in 3.2.

C0urante · 2022-02-28T18:46:53Z

                scanUrlsAndAddPlugins(
                        getParent(),
-                        ClasspathHelper.forJavaClassPath().toArray(new URL[0]),
-                        null


Thanks, the "unused parameter" warning from my IDE has been bugging me here about this for ages 🙏

C0urante · 2022-02-28T19:00:35Z

Noticed that the "resolve comment" option isn't available on this PR, apparently you need to either be the author of a PR or have write access to the repo in order to resolve comments (ref). Feel free to resolve everything from me if you'd like; might help reduce noise on this page for other reviewers.

mimaison · 2022-03-01T20:17:01Z

@rhauch Can you take a look? It would be great to get this in 3.2. Thanks

tombentley

A few nits, but looks good, thanks!

tombentley · 2022-03-02T11:51:39Z

    }

+    @Override
+    public List<ConfigKeyInfo> connectorPluginConfig(String pluginName) {


Should it really be called connectorPluginConfig when it handles other plugins too?

In this context we consider connectors, transforms, converters, etc as "connector plugins" as opposed to "worker plugins" such as config providers and rest extensions.

With that in mind, do you think it's still worth naming this method and ConnectorPluginInfo?

If we continue to put the onus on the Herder to ensure that the requested plugin has a supported type (e.g., throw an error if it's a REST extension), then I think "connector plugin" is useful since it differentiates between worker and connector plugins.

But it looks like the ConnectorPluginInfo class is itself completely agnostic on the connector-plugin vs. worker-plugin front, and it might even be possible to leverage it with no modifications if we eventually decide to add support for exposing worker plugin information to the REST API. So I think renaming that to PluginInfo (or something like that) would be reasonable.

I pushed an update to rename ConnectorPluginInfo to PluginInfo

tombentley · 2022-03-02T11:59:10Z

+        Set<PluginDesc<Connector>> connectors = new TreeSet<>();
+        for (PluginDesc<SinkConnector> sinkConnector : sinkConnectors) {
+            connectors.add((PluginDesc<Connector>) (PluginDesc<? extends Connector>) sinkConnector);
+        }
+        for (PluginDesc<SourceConnector> sourceConnector : sourceConnectors) {
+            connectors.add((PluginDesc<Connector>) (PluginDesc<? extends Connector>) sourceConnector);
+        }
        return connectors;


Suggested change

Set<PluginDesc<Connector>> connectors = new TreeSet<>();

for (PluginDesc<SinkConnector> sinkConnector : sinkConnectors) {

connectors.add((PluginDesc<Connector>) (PluginDesc<? extends Connector>) sinkConnector);

}

for (PluginDesc<SourceConnector> sourceConnector : sourceConnectors) {

connectors.add((PluginDesc<Connector>) (PluginDesc<? extends Connector>) sourceConnector);

}

return connectors;

Set<PluginDesc<Connector>> connectors = new TreeSet<>((Set) sinkConnectors);

connectors.addAll((Set) sourceConnectors);

return connectors;

tombentley · 2022-03-02T12:02:09Z

 public enum PluginType {
    SOURCE(SourceConnector.class),
    SINK(SinkConnector.class),
-    CONNECTOR(Connector.class),


This makes me wonder why CONNECTOR was ever in this enum. Do you know @mimaison / @C0urante ?

To be honest I'm not sure as it wasn't used by anything.

I think it allowed for classes that subclassed Connector but didn't subclass from SinkConnector or SourceConnector to still be detected by the worker during plugin scanning and included in responses for the /connector-plugins endpoint, with a type of UNKNOWN.

With this change, those kinds of connector classes won't appear in the response for this endpoint anymore.

I think this should be fine. As of #2604, which was merged over a year ago, it's impossible to create that kind of connector anyways.

tombentley · 2022-03-02T12:05:03Z


 import java.util.Objects;

 public class ConnectorPluginInfo {


Should the name remain as ConnectorPluginInfo when it's no longer just for connector plugins?

tombentley · 2022-03-02T12:08:52Z

+        synchronized (this) {
+            if (connectorsOnly) {
+                Set<String> types = new HashSet<>(Arrays.asList(PluginType.SINK.toString(), PluginType.SOURCE.toString()));
+                return Collections.unmodifiableList(connectorPlugins.stream().filter(p -> types.contains(p.type())).collect(Collectors.toList()));


Is it worth using Set.contains for this, rather than .equals and ||?

Not really, I updated the logic to use equals and ||

mimaison · 2022-03-02T17:07:19Z

Thanks @tombentley for the review! I've pushed an update.

tombentley

LGTM, thanks @mimaison!

mimaison · 2022-03-03T13:29:12Z

Thanks @C0urante and @tombentley for the reviews!

mimaison force-pushed the kip-769 branch 2 times, most recently from dac03c6 to 62dd215 Compare December 13, 2021 17:56

mimaison added connect kip Requires or implements a KIP labels Dec 13, 2021

mimaison force-pushed the kip-769 branch 3 times, most recently from feda75b to 10e4b2f Compare February 21, 2022 21:39

KAFKA-13510: Connect APIs to list all connector plugins and retrieve …

df241b8

…their configdefs Implements KIP-769: https://cwiki.apache.org/confluence/display/KAFKA/KIP-769%3A+Connect+APIs+to+list+all+connector+plugins+and+retrieve+their+configuration+definitions

mimaison force-pushed the kip-769 branch from 10e4b2f to df241b8 Compare February 21, 2022 21:44

mimaison marked this pull request as ready for review February 21, 2022 21:46

C0urante reviewed Feb 25, 2022

View reviewed changes

Address feedback

57fcf2a

Some more cleanups

79cd5f5

C0urante reviewed Feb 25, 2022

View reviewed changes

Updates following reviews

db234a6

C0urante approved these changes Feb 28, 2022

View reviewed changes

tombentley reviewed Mar 2, 2022

View reviewed changes

Address comment

9ec8020

Rename ConnectorPluginInfo to PluginInfo

1913e71

tombentley approved these changes Mar 3, 2022

View reviewed changes

mimaison merged commit 029a14b into apache:trunk Mar 3, 2022

mimaison deleted the kip-769 branch March 3, 2022 13:28

yashmayya mentioned this pull request Aug 8, 2023

Kafka-13158 Replace EasyMock and PowerMock with Mockito for ConnectClusterStateImpl Test and ConnectorPluginsResourceTest #11167

Closed

3 tasks

Conversation

mimaison commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

mimaison commented Feb 23, 2022

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimaison commented Feb 25, 2022

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mimaison commented Feb 28, 2022

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimaison commented Mar 1, 2022

Uh oh!

tombentley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison commented Mar 2, 2022

Uh oh!

tombentley left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Mar 3, 2022

mimaison commented Dec 7, 2021 •

edited

Loading

C0urante commented Feb 28, 2022 •

edited

Loading

C0urante Mar 2, 2022 •

edited

Loading