You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "C0urante (via GitHub)" <gi...@apache.org> on 2023/01/23 16:32:48 UTC
[GitHub] [kafka] C0urante opened a new pull request, #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
C0urante opened a new pull request, #13148:
URL: https://github.com/apache/kafka/pull/13148
[Jira](https://issues.apache.org/jira/browse/KAFKA-14645)
If we don't switch to the classloader of a plugin before loading its `ConfigDef`, then classloading bugs can appear for, e.g., properties with the `CLASS` type. See https://github.com/kcctl/kcctl/issues/266 for an instance of this kind of bug.
This PR adds a classloader swap in the part of the code base that's responsible for servicing requests to the `GET /connector-plugins/<type>/config` endpoint.
The existing monolithic unit test for this logic is broken out into dedicated individual unit tests for each kind of connector plugin; this is done to avoid having to reset expectations on the mocked `Plugins` object when verifying calls to `withClassLoader`, since in the unit testing environment the same classloader may be used for multiple different plugin types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13148:
URL: https://github.com/apache/kafka/pull/13148#issuecomment-1408822680
@mimaison Yeah, that's correct. I'm hoping that the `verify` calls for `plugins::withClassLoader` will be sufficient for now. I've also added a small comment explaining why we perform that check, which hopefully makes this part a little clearer and harder to break with future changes.
We could file a follow-up ticket to add true integration or system tests for isolated classpath loading that set up plugins off of the system classpath and test various APIs that interact with them (such as the `GET /connector-plugins/<type>/config` endpoint), but I'm on the fence as to whether that'd be worth the bloat in CI build time and testing logic. LMKWYT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13148:
URL: https://github.com/apache/kafka/pull/13148#issuecomment-1409001897
Thanks Mickael!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13148:
URL: https://github.com/apache/kafka/pull/13148#issuecomment-1408944140
Thanks @C0urante, I just wanted to clarify this was indeed the intent. Like you, I don't think we necessarily need system tests for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13148:
URL: https://github.com/apache/kafka/pull/13148#issuecomment-1408432038
Thanks for the PR @C0urante ! I think none of the tests actually reproduce the issue, I can get them to pass (minus the verify for the new methods) with the previous logic. However I don't think that prevents us from merging this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13148:
URL: https://github.com/apache/kafka/pull/13148#discussion_r1084283540
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/AbstractHerder.java:
##########
@@ -821,13 +827,14 @@ public List<ConfigKeyInfo> connectorPluginConfig(String pluginName) {
default:
throw new BadRequestException("Invalid plugin type " + pluginType + ". Valid types are sink, source, converter, header_converter, transformation, predicate.");
}
- } catch (ClassNotFoundException cnfe) {
- throw new NotFoundException("Unknown plugin " + pluginName + ".");
- }
- for (ConfigDef.ConfigKey configKey : configDefs.configKeys().values()) {
- results.add(AbstractHerder.convertConfigKey(configKey));
+ List<ConfigKeyInfo> results = new ArrayList<>();
+ for (ConfigDef.ConfigKey configKey : configDefs.configKeys().values()) {
+ results.add(AbstractHerder.convertConfigKey(configKey));
+ }
+ return results;
+ } catch (ClassNotFoundException e) {
+ throw new ConnectException("Failed to load plugin class or one of its dependencies", e);
Review Comment:
IMO an HTTP 500 response (which will be the result of throwing a `ConnectException` here) is more warranted in this place since we've already checked to see that the plugin type is recognized on the worker by this point. If things break here, it's more likely that the worker or one of its plugins is set up or packaged incorrectly than that the user issued a bad request.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #13148: KAFKA-14645: Use plugin classloader when retrieving connector plugin config definitions
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #13148:
URL: https://github.com/apache/kafka/pull/13148
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org