You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/03/30 16:51:43 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2601: Add exception to catch class loading error
milleruntime opened a new pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601
I am not sure if this is a bug or not. I found a place in the code where `context` is null but we are trying to use the Accumulo Class Loader.
--
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 pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1085420050
> > So, I went back and looked at some code, and I believe that when useAccumuloClassLoader is false, it means to load the class from the (Client) VMs classpath. Client is in parens here because I believe it's a distinction between server side and client side use of the code
>
> I think that is correct that it is for loading on the client vs server side. On the client side we don't want to do anything special when loading classes, just use standard java mechanisms.
I think this idea has some merit. But, I think it will be much simpler to address class loading changes further *after* the legacy stuff is removed, which can be done in 3.0.
--
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 #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083642007
> So, I went back and looked at some code, and I believe that when useAccumuloClassLoader is false, it means to load the class from the (Client) VMs classpath. Client is in parens here because I believe it's a distinction between server side and client side use of the code
I think that is correct that it is for loading on the client vs server side. On the client side we don't want to do anything special when loading classes, just use standard java mechanisms.
--
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] milleruntime closed pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
milleruntime closed pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601
--
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] milleruntime commented on pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083646895
OK so it sounds like this is not a bug, just confusing, complicated code. I will close this PR since the error it is exposing is not an error.
--
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] milleruntime commented on pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083650459
> Reading this makes me wonder if moving classloading into the context would simplify things. The ClientContext could load classses using standard java mechanisms and the ServerContext could override that behaviour and load classes using the specialized Accumulo mechanisms. Then maybe would not need the `useAccumuloClassLoader(boolean)` method.
That would be nice to differentiate between the client and server code since that is obfuscated right now. When I think of iterators, I think of server behavior but we have a lot of client code to deal with iterators.
--
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] milleruntime commented on pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083419697
I talked to @dlmarion a bit to try and figure out if we need this check or not. The problem I am having is there are parts of the code where we set useAccumuloClassLoader = true but context = null. The class loader code has a guard against context being null (since it is optional) and will default to returning the VFS loader. Table context is optional for users to set but internally this seems like a bug to me since we are expecting to use the "AccumuloClassLoader" to be returned but return the deprecated `AccumuloVFSClassLoader`. It could just be confusion over what exactly is the "AccumuloClassLoader".
Here is the part that returns the class loader in ClassLoaderUtil:
https://github.com/apache/accumulo/blob/68acf9f101efeaf3a612b39e898a3a58090470fa/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java#L75-L81
--
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 #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083645481
Reading this makes me wonder if moving classloading into the context would simplify things. The ClientContext could load classses using standard java mechanisms and the ServerContext could override that behaviour and load classes using the specialized Accumulo mechanisms. Then maybe would not need the `useAccumuloClassLoader(boolean)` method.
--
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] milleruntime edited a comment on pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
milleruntime edited a comment on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083453657
Here is the places that iterator methods use the method `useAccumuloClassLoader(boolean)`
- ClientSideIteratorScanner.iterator().iterEnv(env).useAccumuloClassLoader(false);
- RFileScanner.iterator() -> il.iterEnv(new IterEnv()).useAccumuloClassLoader(true));
- RFileScanner.iterator() -> iterEnv(new IterEnv()).useAccumuloClassLoader(false));
- OfflineIterator -> createIterator(KeyExtent, Collection<StoredTabletFile>) -> iterLoad.iterEnv(iterEnv).useAccumuloClassLoader(false)));
- IterConfigUtil.convertItersAndLoad() -> il = il.iterEnv(env).useAccumuloClassLoader(true)...
- IterConfigUtilTest.createIter() -> iterLoad.iterEnv(new DefaultIteratorEnvironment(conf)).useAccumuloClassLoader(true);
- CollectTabletStats -> createScanIterator() -> IterConfigUtil.loadIterators(visFilter, il.useAccumuloClassLoader(true));
- ConditionCheckerContext -> buildIterator().iterEnv(tie).useAccumuloClassLoader(true)...
- ScanDataSource -> createIterator().useAccumuloClassLoader(true).context(context);
--
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 edited a comment on pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
dlmarion edited a comment on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083468315
So, I went back and looked at some code, and I believe that when `useAccumuloClassLoader` is false, it means to load the class from the (Client) VMs classpath. Client is in parens here because I believe it's a distinction between server side and client side use of the code. If you look at the client-side references (ClientSideIteratorScanner, OfflineIterator, RFileScanner) to `IterLoad.useAccumuloClassLoader`, in all but one case they set it to `false` meaning use the client VM classloader (reference IterConfigUtil.loadClass).
The context is just a the name of a configured classloader and is only supported on the server side. If context is null, then classes are loaded from the JVM application classloader. If context is not null, then classes are loaded from the classloader associated with that context name.
> The problem I am having is there are parts of the code where we set useAccumuloClassLoader = true but context = null.
I don't think this is a problem. This is just saying that you want to load a class from the application classloader
> Table context is optional for users to set but internally this seems like a bug to me since we are expecting to use the "AccumuloClassLoader" to be returned but return the deprecated AccumuloVFSClassLoader
The deprecated AccumuloVFSClassLoader is effectively the application classloader. I don't think this is a bug.
--
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 #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083654104
> That would be nice to differentiate between the client and server code since that is obfuscated right now. When I think of iterators, I think of server behavior but we have a lot of client code to deal with iterators.
I'm not sure, but this problem may be more general than iterators. There maybe classes that other than iterators that are loaded in the client code.
--
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] milleruntime commented on pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083453657
Here is the places that iterator methods use `useAccumuloClassLoader(boolean)`
- ClientSideIteratorScanner.iterator().iterEnv(env).useAccumuloClassLoader(false);
- RFileScanner.iterator() -> il.iterEnv(new IterEnv()).useAccumuloClassLoader(true));
- RFileScanner.iterator() -> iterEnv(new IterEnv()).useAccumuloClassLoader(false));
- OfflineIterator -> createIterator(KeyExtent, Collection<StoredTabletFile>) -> iterLoad.iterEnv(iterEnv).useAccumuloClassLoader(false)));
- IterConfigUtil.convertItersAndLoad() -> il = il.iterEnv(env).useAccumuloClassLoader(true)...
- IterConfigUtilTest.createIter() -> iterLoad.iterEnv(new DefaultIteratorEnvironment(conf)).useAccumuloClassLoader(true);
- CollectTabletStats -> createScanIterator() -> IterConfigUtil.loadIterators(visFilter, il.useAccumuloClassLoader(true));
- ConditionCheckerContext -> buildIterator().iterEnv(tie).useAccumuloClassLoader(true)...
- ScanDataSource -> createIterator().useAccumuloClassLoader(true).context(context);
--
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 pull request #2601: Add exception to catch class loading error
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2601:
URL: https://github.com/apache/accumulo/pull/2601#issuecomment-1083468315
So, I went back and looked at some code, and I believe that when `useAccumuloClassLoader` is false, it means to load the class from the (Client) VMs classpath. Client is in parens here because I believe it's a distinction between server side and client side use of the code. If you look at the client-side references (ClientSideIteratorScanner, OfflineIterator, RFileScanner) to `IterLoad.useAccumuloClassLoader`, in all but one case they set it to `false` meaning use the client VM classloader (reference IterConfigUtil.loadClass).
The context is just a the name of a configured classloader. If context is null, then classes are loaded from the JVM application classloader. If context is not null, then classes are loaded from the classloader associated with that context name.
> The problem I am having is there are parts of the code where we set useAccumuloClassLoader = true but context = null.
I don't think this is a problem. This is just saying that you want to load a class from the application classloader
> Table context is optional for users to set but internally this seems like a bug to me since we are expecting to use the "AccumuloClassLoader" to be returned but return the deprecated AccumuloVFSClassLoader
The deprecated AccumuloVFSClassLoader is effectively the application classloader. I don't think this is a bug.
--
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