refactor: remove usage of the org.apache.kafka.connect.runtime library #9561
Conversation
There was a problem hiding this comment.
Thanks @jzaralim ! LGTM with a few things inline.
Also, can you run a quick manual check that LIST CONNECTOR PLUGINS; works as expected with these changes, in the case where at least one plugin is returned? Our integration test coverage for connector functionality only has a test case where no connector plugins are expected to be returned since it was tricky to add a connector plugin for the test.
| private final ConnectorType type; | ||
|
|
||
| @JsonCreator | ||
| public ConnectorInfo( |
There was a problem hiding this comment.
Why did tasks get removed from this? This class appears to be user-facing, since it's returned from the response of CREATE CONNECTOR. It'd be good to go through the other user-facing classes to double-check that those aren't dropping fields as well, since we don't know how users might be using these additional fields today.
There was a problem hiding this comment.
Added all the removed fields back.
There was a problem hiding this comment.
Some of them aren't user-facing, right? It looks like ConfigInfos might only be used internally, in which case it's fine to leave out the extra fields from that. It's just the user-facing ones like ConnectorInfo that it'd be good to keep extra fields for, for purposes of backwards compatibility.
ksqldb-rest-model/src/main/java/io/confluent/ksql/rest/entity/SimpleConnectorPluginInfo.java
Show resolved
Hide resolved
| @@ -701,11 +702,11 @@ public void shouldPrintConnectorPluginsList() { | |||
| + " \"warnings\" : [ ]," + NEWLINE | |||
| + " \"connectorsPlugins\" : [ {" + NEWLINE | |||
| + " \"className\" : \"clazz1\"," + NEWLINE | |||
| + " \"type\" : \"source\"," + NEWLINE | |||
| + " \"type\" : \"SOURCE\"," + NEWLINE | |||
There was a problem hiding this comment.
How come this is uppercased now? It looks like the new PluginType class that got copied over also has toLowerCase() in its toString() method.
There was a problem hiding this comment.
PluginType was missing the JsonValue annotation so it wasn't even calling toString 🤦 fixed
...b-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/ListConnectorsExecutor.java
Outdated
Show resolved
Hide resolved
...b-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/ListConnectorsExecutor.java
Outdated
Show resolved
Hide resolved
| import java.util.Objects; | ||
| import org.apache.kafka.connect.runtime.rest.entities.ConnectorType; | ||
| import org.apache.kafka.common.config.provider.ConfigProvider; | ||
| import org.apache.kafka.connect.connector.policy.ConnectorClientConfigOverridePolicy; |
There was a problem hiding this comment.
Are there follow-up plans to remove these non-runtime usages of connect libraries as well, or was it just the runtime ones which were causing trouble?
There was a problem hiding this comment.
No, just these ones. Most of the other connect libraries would either be too complicated to remove (the connect executable) or are risky wrt backwards compatibility (related to serialization).
Description
org.apache.kafka.connect.runtimeis an internal library so we should avoid using it. There was a previous attempt to do this in #8990, but it was closed because we were lacking integration tests. They got added in #9500 , so we're revisiting this.Testing done
Integration tests still pass
Reviewer checklist