feat: back out Connect auto-import feature #3386
Conversation
|
Boo! What is our approach to actually providing this as an integrated solution then? We are planning on having proper auto-import right? Otherwise I'd say it will still feel like too products and not the integrated solution we were targeting. |
ksql-cli/src/main/java/io/confluent/ksql/cli/console/Console.java
Outdated
Show resolved
Hide resolved
| sources | ||
| sources, | ||
| topics, | ||
| warnings |
There was a problem hiding this comment.
Looks like ConnectorDescription is only ever created with either the topics list or a single warning.
Might be worth codifying this. i.e. the topics param passed in to ConnectorDescription should be a union type of either the topic list or a single warning.
There was a problem hiding this comment.
In the future we might want to supply other warnings that are unrelated to the list of topics (e.g. failed to connect to Connect) - I don't think that coupling necessarily makes sense.
| return ImmutableList.of(); | ||
| } | ||
|
|
||
| return admin.listTopics().names().get(5, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Consider making this timeout configurable?
There was a problem hiding this comment.
haha calling me out on indolence.
There was a problem hiding this comment.
So I looked into this a little bit, we don't have any configureable timeouts at the moment (in KsqlConfig anyway). I'm going to follow this PR with one that adds a DEFAULT_EXTERNAL_SERVICE_TIMEOUT and then we can add specific overrides for it.
We need to figure out how to do this without providing special support in KSQL for particular connectors. We don't want to be in the business of having to implement particular interfaces for every connector out there and always playing catch-up. |
|
Building off of what @MichaelDrogalis said, it's not just per-connector things (even after this revert we still have per-connector things that we'll need to remove) it's that the experience around auto-import isn't great and extremely limited, and the overhead of typing one |
Description
After discussions with @derekjn and @MichaelDrogalis we think it's probably better to back out the connect auto-import. Instead, describing a supported connector will list the topics available (see unit tests).
Testing done
Reviewer checklist