You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2023/03/03 01:17:44 UTC
[GitHub] [kafka] gharris1727 opened a new pull request, #13334: MINOR: Move plugin path parsing from DelegatingClassLoader to PluginUtils
gharris1727 opened a new pull request, #13334:
URL: https://github.com/apache/kafka/pull/13334
The logic for decomposing a `plugin.path` list into the component single-plugin locations is nontrivial, and currently implemented by the DelegatingClassLoader. Decoupling this logic from the DelegatingClassLoader will allow us to reuse these components in the KIP-898 connect-plugin-path.sh.
DelegatingClassLoader now takes a List<Path>, each corresponding exactly to one pluginLocation, which I'm currently trying to standardize as the name of the on-disk location which is assigned exactly one PluginClassLoader.
Conflicts with #13333 which also changes the type of PLUGIN_JARS.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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] gharris1727 commented on a diff in pull request #13334: MINOR: Move plugin path parsing from DelegatingClassLoader to PluginUtils
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13334:
URL: https://github.com/apache/kafka/pull/13334#discussion_r1205901798
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java:
##########
@@ -188,11 +191,34 @@ public static boolean isClassFile(Path path) {
return path.toString().toLowerCase(Locale.ROOT).endsWith(".class");
}
- public static List<Path> pluginLocations(Path topPath) throws IOException {
+ public static List<Path> pluginLocations(String pluginPath) {
+ if (pluginPath == null) {
+ return Collections.emptyList();
+ }
+ String[] pluginPathElements = COMMA_WITH_WHITESPACE.split(pluginPath.trim(), -1);
+ List<Path> pluginLocations = new ArrayList<>();
+ for (String path : pluginPathElements) {
+ try {
+ Path pluginPathElement = Paths.get(path).toAbsolutePath();
+ // Currently 'plugin.paths' property is a list of top-level directories
+ // containing plugins
+ if (Files.isDirectory(pluginPathElement)) {
+ pluginLocations.addAll(pluginLocations(pluginPathElement));
+ } else if (isArchive(pluginPathElement)) {
+ pluginLocations.add(pluginPathElement);
+ }
Review Comment:
Thanks for catching that, the Paths.get() throws this exception but it's not checked.
I'm not sure if the catch in initLoaders still needs the InvalidPathException, but I left it in just to be safe.
--
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 #13334: MINOR: Move plugin path parsing from DelegatingClassLoader to PluginUtils
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13334:
URL: https://github.com/apache/kafka/pull/13334#discussion_r1207028289
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java:
##########
@@ -188,11 +191,34 @@ public static boolean isClassFile(Path path) {
return path.toString().toLowerCase(Locale.ROOT).endsWith(".class");
}
- public static List<Path> pluginLocations(Path topPath) throws IOException {
+ public static List<Path> pluginLocations(String pluginPath) {
+ if (pluginPath == null) {
+ return Collections.emptyList();
+ }
+ String[] pluginPathElements = COMMA_WITH_WHITESPACE.split(pluginPath.trim(), -1);
+ List<Path> pluginLocations = new ArrayList<>();
+ for (String path : pluginPathElements) {
+ try {
+ Path pluginPathElement = Paths.get(path).toAbsolutePath();
+ // Currently 'plugin.paths' property is a list of top-level directories
+ // containing plugins
+ if (Files.isDirectory(pluginPathElement)) {
+ pluginLocations.addAll(pluginLocations(pluginPathElement));
+ } else if (isArchive(pluginPathElement)) {
+ pluginLocations.add(pluginPathElement);
+ }
Review Comment:
👍 Thanks, LGTM
--
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 #13334: MINOR: Move plugin path parsing from DelegatingClassLoader to PluginUtils
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on code in PR #13334:
URL: https://github.com/apache/kafka/pull/13334#discussion_r1205854894
##########
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/PluginUtils.java:
##########
@@ -188,11 +191,34 @@ public static boolean isClassFile(Path path) {
return path.toString().toLowerCase(Locale.ROOT).endsWith(".class");
}
- public static List<Path> pluginLocations(Path topPath) throws IOException {
+ public static List<Path> pluginLocations(String pluginPath) {
+ if (pluginPath == null) {
+ return Collections.emptyList();
+ }
+ String[] pluginPathElements = COMMA_WITH_WHITESPACE.split(pluginPath.trim(), -1);
+ List<Path> pluginLocations = new ArrayList<>();
+ for (String path : pluginPathElements) {
+ try {
+ Path pluginPathElement = Paths.get(path).toAbsolutePath();
+ // Currently 'plugin.paths' property is a list of top-level directories
+ // containing plugins
+ if (Files.isDirectory(pluginPathElement)) {
+ pluginLocations.addAll(pluginLocations(pluginPathElement));
+ } else if (isArchive(pluginPathElement)) {
+ pluginLocations.add(pluginPathElement);
+ }
Review Comment:
Do we also want to include the catch block for `InvalidPathException` here, which matches the [logic on trunk](https://github.com/apache/kafka/blob/dc00832b965c29fbbb4148cc680fadfc3f28642a/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/isolation/DelegatingClassLoader.java#L266-L267)?
--
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 merged pull request #13334: MINOR: Move plugin path parsing from DelegatingClassLoader to PluginUtils
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante merged PR #13334:
URL: https://github.com/apache/kafka/pull/13334
--
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