feat(java client): Add connector functions to java client #7222

jzaralim · 2021-03-12T22:57:21Z

Description

Adds createConnector, dropConnector, listConnector and describeConnector to the Java client. I can split this into separate PRs if that makes it easier to review.

Testing done

Unit and integration tests

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

… and describeConnector to java client

vcrfxia

Hey @jzaralim -- awesome test coverage! Are you planning to add docs in a separate PR? If you wouldn't mind, we should also add docs for the new serverInfo() method you added as part of the 0.16 release (could be a separate PR).

Left some questions inline, mostly about the interfaces, but the high-level LGTM!

vcrfxia · 2021-03-15T21:07:52Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/CreateConnectorResult.java

+
+import java.util.Map;
+
+public interface CreateConnectorResult {


Why are the methods in this result useful? Doesn't the user already have all this information (since it's required in order to call createConnect() in the first place)?

I'm not sure, but this is the response that Connect returns when creating a new connector. If this is not useful for the Java client, we can remove it.

Hm actually I guess this wouldn't be the only place the client shows less info than the old rest api. I'll remove it, having 3 very similar classes for connector responses is clunky anyway.

vcrfxia · 2021-03-15T22:17:54Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/ConnectorDescription.java

+
+  /**
+   *
+   * @return a list of sources that this connector reads/writes to


What sources are these? External sources (as opposed to ksqlDB streams/tables)?

Updated the Javadoc to make this more clear.

vcrfxia · 2021-03-15T22:18:22Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/ConnectorDescription.java

+
+  /**
+   *
+   * @return a list of topics consumed by this connector


"consumed" meaning this is only present for sink connectors?

Updated the Javadoc to make this more clear.

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/ConnectorDescription.java

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/ConnectorType.java

vcrfxia · 2021-03-15T22:30:27Z

...-client/src/main/java/io/confluent/ksql/api/client/impl/ConnectorCommandResponseHandler.java

+  private static Optional<List<ConnectorInfo>> getListConnectorsResponse(
+      final JsonObject connectorsEntity) {
+    try {
+      final JsonArray connectors = connectorsEntity.getJsonArray("connectors");


A bit late for this PR but we've been wanting to change the pattern of how the Java client response interfaces relate to the server response objects for a while now, in order to avoid custom JSON parsing like this. Here's the ticket with context: #6042

We should really do this before the next time we add additional interfaces, but it's outside the scope for this PR. (If you're keen on helping clean up the codebase and want to tackle it as a follow-up PR that'd be super, but no pressure 😁 )

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/impl/ConnectorDescriptionImpl.java

vcrfxia · 2021-03-15T22:37:31Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+  @Test
+  public void shouldListConnectors() throws Exception {
+    // Given:
+    makeKsqlRequest("CREATE SOURCE CONNECTOR " + TEST_CONNECTOR + " WITH ('connector.class'='" + MOCK_SOURCE_CLASS + "');");


If all three of these tests create the same connector, why do whichever tests run second and third not fail (because the connector already exists)?

The statement fails but doesn't throw an error. I ended up moving this to setUp and tearDown though.

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/Client.java

vcrfxia

Thanks @jzaralim -- LGTM once the inline comments are addressed and docs are added. It'd be nice to get another opinion on the new client interfaces as well cc @colinhicks .

vcrfxia · 2021-03-17T03:23:41Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/ConnectorType.java

+    SOURCE,
+    SINK,
+    /**
+     * Denotes an unknown connector type. This is used when there were errors in connector creation.


Is this the only reason the connector type might be UNKNOWN or are there others as well, for example, when listing existing connectors? For some reason I thought there were other possible reasons for an UNKNOWN type as well but I could be wrong.

I couldn't find any other references to UNKNOWN, but there could be a reason on the Connect side of things that I'm missing. Updated docs to be more accurate

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

vcrfxia

Two more minor questions inline -- LGTM overall! (Plus the comments from before on docs and getting product input on the new interfaces) Thanks @jzaralim .

vcrfxia · 2021-03-18T21:53:38Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+    assertThatEventually(() -> {
+      try {
+        return client.listConnectors().get().size();
+      } catch (InterruptedException | ExecutionException e) {


Is this exception expected (under normal cases)? What would cause this?

It's not. It's just that since this is a part of a lambda, we need to do a try-catch here in order for the code to compile.

vcrfxia · 2021-03-18T21:53:40Z

...api-client/src/test/java/io/confluent/ksql/api/client/integration/ClientIntegrationTest.java

+        () -> {
+          try {
+            return ((ConnectorList) makeKsqlRequest("SHOW CONNECTORS;").get(0)).getConnectors().size();
+          } catch (AssertionError e) {


Same questions as above. I notice there's no analogous try-catch around the SHOW CONNECTORS; request in the cleanupConnectors() helper method, curious why this situation is different.

It takes a while to provision a connector, even after CREATE CONNECTOR returns a successful response. When ksqlDB calls SHOW CONNECTORS;, it makes a call to '/info' for reach connector, so if the new connector is not done provisioning, it returns an error response which propagates.

derekjn · 2021-03-19T17:13:46Z

ksqldb-api-client/src/main/java/io/confluent/ksql/api/client/Client.java

+   * @return result of connector creation
+   */
+  CompletableFuture<Void> createConnector(
+      String connectorName, boolean isSource, Map<String, String> properties);


I believe we use Map<String, Object> to pass properties elsewhere in the client interface. Is there a reason for using <String, String> here, or should we keep this consistent with the other interface methods?

Good catch! Will update the PR.

feat(java client): Add createConnector, dropConnector, listConnectors…

2b3a202

… and describeConnector to java client

jzaralim requested a review from a team as a code owner March 12, 2021 22:57

checkstyle

e05d133

vcrfxia reviewed Mar 15, 2021

View reviewed changes

Zara Lim added 3 commits March 16, 2021 12:13

address review comments

c5e41d0

remove CreateConnectorResult

6288479

checkstyle

df28b44

vcrfxia approved these changes Mar 17, 2021

View reviewed changes

Zara Lim added 2 commits March 17, 2021 15:18

address review comments

ba0ee36

ensure connectors are cleaned up between tests

22db7fa

vcrfxia reviewed Mar 18, 2021

View reviewed changes

jzaralim requested a review from derekjn March 19, 2021 17:06

derekjn reviewed Mar 19, 2021

View reviewed changes

change properties param map value to object

cfa7367

jzaralim merged commit 7766195 into confluentinc:master Mar 19, 2021

jzaralim deleted the connector-java-client branch March 19, 2021 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(java client): Add connector functions to java client #7222

feat(java client): Add connector functions to java client #7222

jzaralim commented Mar 12, 2021

vcrfxia left a comment

vcrfxia Mar 15, 2021

jzaralim Mar 16, 2021

jzaralim Mar 16, 2021

vcrfxia Mar 15, 2021

jzaralim Mar 16, 2021

vcrfxia Mar 15, 2021

jzaralim Mar 16, 2021

vcrfxia Mar 15, 2021

vcrfxia Mar 15, 2021

jzaralim Mar 16, 2021

vcrfxia left a comment

vcrfxia Mar 17, 2021

jzaralim Mar 17, 2021

vcrfxia left a comment

vcrfxia Mar 18, 2021

jzaralim Mar 18, 2021

vcrfxia Mar 18, 2021

jzaralim Mar 18, 2021

derekjn Mar 19, 2021

jzaralim Mar 19, 2021


		import java.util.Map;

		public interface CreateConnectorResult {

feat(java client): Add connector functions to java client #7222

feat(java client): Add connector functions to java client #7222

Conversation

jzaralim commented Mar 12, 2021

Description

Testing done

Reviewer checklist

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment