-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add DESCRIBE functionality for connectors #3206
Conversation
0d92e93
to
e182ca0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly questions and a few suggestions.
ksql-engine/src/main/java/io/confluent/ksql/services/DefaultConnectClient.java
Outdated
Show resolved
Hide resolved
setupConfigService(); | ||
givenNoMoreRecords(when(consumer.poll(any()))); | ||
givenNoMoreRecords(when(consumer.poll(any())), noMoreLatch); | ||
configService.startAsync().awaitRunning(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Isn't this where the action happens? If so, would this be better suited for "when"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose so! I thought that the "action" happens when stopAsync()
is called (e.g. it calls triggerShutdown
which in turn causes the wakeup/close, but I think it makes sense to put it in "when' as well. Will make that change.
ksql-engine/src/test/java/io/confluent/ksql/services/DefaultConnectClientTest.java
Outdated
Show resolved
Hide resolved
ksql-rest-app/src/main/java/io/confluent/ksql/rest/entity/ConnectorDescription.java
Outdated
Show resolved
Hide resolved
ksql-rest-app/src/main/java/io/confluent/ksql/rest/entity/ConnectorDescription.java
Show resolved
Hide resolved
...app/src/test/java/io/confluent/ksql/rest/server/execution/DescribeConnectorExecutorTest.java
Show resolved
Hide resolved
4e2da68
to
80b0a8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty close to me. I think some doc or other clarification on the output of describe would be super helpful. What do you think?
...app/src/test/java/io/confluent/ksql/rest/server/execution/DescribeConnectorExecutorTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use "KSQL Sources" for now. Otherwise LGTM.
80b0a8f
to
797feaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Implements functionality to describe a connector.
Testing done
Unit testing and local test:
Reviewer checklist