Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
KAFKA-15031: Add plugin.discovery to Connect worker configuration (KIP-898) #14055
KAFKA-15031: Add plugin.discovery to Connect worker configuration (KIP-898) #14055
Changes from 5 commits
9b26287
2a0f702
c92cb3d
266c39f
28e0f50
da71b8d
346e23c
436f595
22cbcd0
e0ccb0c
228ea45
fd2f693
7d65156
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: language
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.
Also, should we include instructions on what users should do in this case? For many of them, this is going to be the first time hearing about new discovery logic; we should make this message as informative as possible if we want them to start to make use of it.
Some options include:
HYBRID_FAIL
, suggest changing the discovery mode if the missing plugins cannot be updatedThere 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.
Also also, if the point about classpath-installed plugins being unaffected by the plugin discovery mode is correct, should we either filter these plugins out from
missingPlugins
or clarify that those plugins will still be usable withSERVICE_LOAD
?I suppose "visible" is accurate in the sense that these plugins won't be listed by the
GET /connector-plugins
endpoint, but that may confuse users if they notice that they're still able to use these plugins in a connector config after switching toSERVICE_LOAD
.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 think to explaining everything in one warning/error message is unreasonable, and linking out to external documentation is the better strategy. The error message is already lengthy enough with each plugin listing.
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.
Linked documentation is added in #14068
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.
Looks great--one more thing: can we make the same tweak to the language here that we did in
WorkerConfig
by replacing "will not be visible" with "may not be usable"?