You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/08/10 19:32:15 UTC
[GitHub] [accumulo] keith-turner opened a new pull request, #3683: adds validation of classloader context
keith-turner opened a new pull request, #3683:
URL: https://github.com/apache/accumulo/pull/3683
This adds validation of the classloader context without adding a new SPI method and without changing the existing classloading behavior in a bug fix release.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#issuecomment-1673928306
> These changes look okay - but I am lost were these fit into relation to the other, similar changes (https://github.com/apache/accumulo/pull/3682 in particular)
@EdColeman I added a comment on #3682
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290684472
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
I don't think it possible to avoid bad config being set. We could do complex validation of any classloader config set via ZK that exmanines the new config against all existing classloader config. However since the config could be set in a file, its still possible to set bad config.
Its way out of scope of this PR which is focused on validation of a single config, but maybe we could create something like the following for loading table plugins. After creating the following utility method somewhere we could then adapt code that instantiates classes to use the new utility method.
```java
class SomeUtilClass {
/**
* Creates a new instance of an object using a tables context classloader retrying until the object is created.
* /
T newInstance(TableConfiguration config, Property prop, Class<T> expectedType){
// TODO use retry
while(true){
try{
String context = config.getClassloaderContext();
String clazz = config.getClassName(prop);
return instantiateClass(context, clazz, expectedType);
} catch(Exception e){
// assume this is caused by bad classloader config and retry
// TODO log message and sleep using retry
}
}
}
}
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3683: adds validation of classloader context
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290698383
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
The original intent was to validate that `table.class.loader.context` is a valid context name in the configuration. Valid, I think, means that the ClassLoaderContextFactory implementation can resolve the name and construct a ClassLoader for it. Validating the classpath for that context is beyond the scope.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291506996
##########
start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java:
##########
@@ -158,14 +158,6 @@ public synchronized void setContextConfig(ContextsConfig config) {
this.config = config;
}
- public boolean isKnownContext(String contextName) {
Review Comment:
I removed public from the class
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290684472
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
I don't think it possible to avoid bad config being set. We could do complex validation of any classloader config set via ZK that exmanines the new config against all existing classloader config. However since the config could be set in a file, its still possible to set bad config.
Its way out of scope of this PR which is focused on validation of a single config, but maybe we could create something like the following for loading table plugins. After creating the following utility method somewhere we could then adapt code that instantiates classes to use the new utility method.
```java
class SomeUtilClass {
/**
* Creates a new instance of an object using a tables context classloader retrying until the object is created.
* /
T newInstance(TableConfiguration config, Property prop, Class<T> expectedType){
// TODO use retry
while(true){
try{
String context = config.getClassloaderContext();
String clazz = config.getClassName(prop);
return instantiateClass(context, clazz, expectedType);
} catch(Exception e){
// assume this is caused by bad classloader config and retry, once the user fixes the config it will all work
// TODO log message and sleep using retry
}
}
}
}
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290697927
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
> Or have I lost the thread on this and misunderstanding the scope of the check and what we are attempting to protect against?
I am trying to achieve the goal that is described in the one line description of #3678 which is : `Validate that classloader context is valid when property is set`. I think that is a narrowly defined and very useful goal and that these change achieve the goal.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3683: adds validation of classloader context
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290698383
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
The original intent was to validate that `table.class.loader.context` is a valid context name in the configuration. Valid, I think, means that the ClassLoaderContextFactory implementation to resolve the name and construct a ClassLoader for it. Validating the classpath for that context is beyond the scope.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3683: adds validation of classloader context
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290632363
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
Review Comment:
This check was removed in #3678 based on @ctubbsii comments [here](https://github.com/apache/accumulo/pull/3678#discussion_r1285091831). We'll have to reconcile the difference of opinion I think.
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
+ } else {
return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
}
}
public static boolean isValidContext(String context) {
Review Comment:
`isValidContext` is only called from `PropUtil.validateProperties`. What you have below is very similar to my [original](https://github.com/apache/accumulo/pull/3678/commits/03f0b362463cdb2d17233a7928fd7efee0fbb518#diff-10adcecab49078fcfd1a0694c44f8f67fb0bbcc45bfe89bc624b794de480e3fdR68) commit in #3678. @ctubbsii comments on that are [here](https://github.com/apache/accumulo/pull/3678#pullrequestreview-1563829134). Again, I think we'll need to reconcile the difference of opinion.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3683: adds validation of classloader context
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290650278
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
IIRC @EdColeman had commented to me that only checking when the property is set could also lead to an issue if the context definition / configuration changes afterwards. That is why I'm also checking the validity of context name in getClassLoader in #3678 .
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290684472
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
I don't think it possible to avoid bad config being set. We could do complex validation of any classloader config set via ZK that exmanines the new config against all existing classloader config. However since the config could be set in a file, its still possible to set bad config.
Its way out of scope of this PR which is focused on validation of a single config, but maybe we could create something like the following for loading table plugins. After creating the following utility method somewhere we could then adapt code that instantiates classes to use the new utility method.
```java
class SomeUtilClass {
/**
* Creates a new instance of an object using a tables context classloader retrying until the object is created.
* /
T newInstance(TableConfiguration config, Property prop, Class<T> expectedType){
// TODO use retry
while(true){
try{
String context = config.getClassloaderContext();
String clazz = config.getClassName(prop);
return instantiateClass(context, clazz);
} catch(Exception e){
// assume this is caused by bad classloader config and retry
// TODO log message and sleep using retry
}
}
}
}
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#issuecomment-1675142797
I created two commits (a revert and then the changes) and pushed those to the 2.1 branch manually (can not do a fast forward merge in GH).
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291509571
##########
core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java:
##########
@@ -102,20 +101,4 @@ public ClassLoader getClassLoader(String contextName) {
return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader
.getContextClassLoader(contextName);
}
-
Review Comment:
I backed out those changes. I looked at the diffs with 26a71d062702ad703f445ab144880c4c0e323c6e and I didn't see anything else that needed to be reverted. I don't think it would be easier to revert the 3678 commit at this point because there is a still config and test changes needed from it.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3683: adds validation of classloader context
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290661094
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
Yes - one of the issues is that the way the context is being used, there have been "bad changes" where contexts that are in use have been deleted / removed. So what was once valid when the context was set, it becomes invalid at some point.
It may be that we can only do so much to insure that a context is valid and remains valid for the lifetime of its usage. If it was correct when set, then it may not be a big leap to assume that it can be kept that way.
However, protecting the system in the face of context errors so that it does not put things into an unrecoverable position should be a design goal. The errors could be because of the user, or maybe a system / component issue - I think that is the path that the PR for preventing half-dead tservers moves in that direction and may be out of the scope of this PR?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290659206
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
Review Comment:
> This check was removed in https://github.com/apache/accumulo/pull/3678 based on @ctubbsii comments https://github.com/apache/accumulo/pull/3678#discussion_r1285091831. We'll have to reconcile the difference of opinion I think.
Thinking about that and this PR, there are two things I like about this PR.
* It follows a principal of least surprise in a bug fix release. It does not change behavior of existing code in subtle ways and add new SPIs.
* There is no scope creep in the solution. Its a very targeted fix to the problem of the context not being verified when its set as property.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3683: adds validation of classloader context
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291564380
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
+ } else {
return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
}
}
public static boolean isValidContext(String context) {
Review Comment:
I'm fine with avoiding the SPI change, with the simpler implementation.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3683: adds validation of classloader context
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291244666
##########
core/src/test/java/org/apache/accumulo/core/classloader/URLClassLoaderFactory.java:
##########
@@ -54,19 +54,4 @@ public ClassLoader getClassLoader(String contextName) {
throw new IllegalArgumentException("Error creating URL from contextName: " + contextName, e);
}
}
-
Review Comment:
There were other changes in URLClassLoaderFactory as part of 3678 that could be backed out, might be easier to revert.
##########
core/src/main/java/org/apache/accumulo/core/classloader/DefaultContextClassLoaderFactory.java:
##########
@@ -102,20 +101,4 @@ public ClassLoader getClassLoader(String contextName) {
return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader
.getContextClassLoader(contextName);
}
-
Review Comment:
With your change in ClassLoaderUtil above, which essentially reverts the changes in #3678, I think you can remove the contextName checks in getClassLoader (lines 98-100). They were added in #3678. I wonder if it would be easier to revert #3678 and then apply the changes in ShellServerIT, PropUtil, SystemPropUtil, and ClassLoaderUtil.
##########
start/src/main/java/org/apache/accumulo/start/classloader/vfs/ContextManager.java:
##########
@@ -158,14 +158,6 @@ public synchronized void setContextConfig(ContextsConfig config) {
this.config = config;
}
- public boolean isKnownContext(String contextName) {
Review Comment:
The `public` modifier was added to the class in #3678 , that could be backed out as well.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291507649
##########
core/src/test/java/org/apache/accumulo/core/classloader/URLClassLoaderFactory.java:
##########
@@ -54,19 +54,4 @@ public ClassLoader getClassLoader(String contextName) {
throw new IllegalArgumentException("Error creating URL from contextName: " + contextName, e);
}
}
-
Review Comment:
I reverted this class back to what it was before 3678.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3683: adds validation of classloader context
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291571186
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
Review Comment:
I think this is fine to keep... but the javadoc on the factory should be updated to manage expectations that the factory may not be called under these circumstances.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3683: adds validation of classloader context
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1291611501
##########
core/src/main/java/org/apache/accumulo/core/spi/common/ContextClassLoaderFactory.java:
##########
@@ -61,7 +61,9 @@ default void init(ContextClassLoaderEnvironment env) {}
* not supported or fails to be constructed.
*
* @param contextName the name of the context that represents a class loader that is managed by
- * this factory (can be null)
+ * this factory. Currently, Accumulo will only call this method for non-null and non-empty
+ * context. For empty or null context, Accumulo will use the system classloader w/o
Review Comment:
```suggestion
* context. For empty or null context, Accumulo will use the system classloader without
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #3683: adds validation of classloader context
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #3683:
URL: https://github.com/apache/accumulo/pull/3683
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a diff in pull request #3683: adds validation of classloader context
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3683:
URL: https://github.com/apache/accumulo/pull/3683#discussion_r1290691049
##########
core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java:
##########
@@ -75,22 +75,29 @@ static synchronized void resetContextFactoryForTests() {
@SuppressWarnings("deprecation")
public static ClassLoader getClassLoader(String context) {
- try {
- if (FACTORY.isValid(context)) {
- return FACTORY.getClassLoader(context);
- } else {
- return org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassLoader();
- }
- } catch (RuntimeException e) {
+ if (context != null && !context.isEmpty()) {
+ return FACTORY.getClassLoader(context);
Review Comment:
I agree with `I don't think it possible to avoid bad config being set` - but I though these changes where an attempt to validate that, at a minimum an existing path was set?
One complication (of many) is that the classpath maybe a regex, that may or may not contain the needed jars / classes - it is only when the use code tries to access a class that it is know to be present. There is no way for our general code to know which jars / classes will be needed / used at any given point by the users.
Or have I lost the thread on this and misunderstanding the scope of the check and what we are attempting to protect against?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org