You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/03/31 16:51:14 UTC
[GitHub] [solr] HoustonPutman opened a new pull request #783: SOLR-16131: Fix class loader for jdbcStream
HoustonPutman opened a new pull request #783:
URL: https://github.com/apache/solr/pull/783
https://issues.apache.org/jira/browse/SOLR-16131
@joel-bernstein can't add you as a reviewer
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840072555
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
> Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
More than using any particular ClassLoader, I was looking to suggest a comment to defend, support, and increase maintainability for future devs. This stuff often relies on a lot of pre-developed knowledge and new time thinking and tinkering, rough for anyone done the line to have to face fresh and unaware again.
> Unfortunately we can't use SolrResourceLoader here ...
I wasn't suggesting using SolrResourceLoader. Regardless of another ClassLoaders existence, that preference of avoidance exists. That's why it seems useful to leave some bread crumbs when/if it's used for the future.
Even with no SolrResourceLoader, there is a hierarchy of ClassLoaders in a ServletContainer and I imagine in some other environments as well. This stuff is tricky even out of the box with the default grab bag of System, WebContainer, and Webapp classloaders float around. When you mix in the different and surprising ways that static stuff can end up initialized, using the class ClassLoader is just expert-oriented stuff.
WebApps (and some other frameworks I'm sure) try and lend a hand by setting you up with the per-thread context class loader. You don't want to have to guess or have it swapped around on you about whether you are loading with the container wide classloader or the webapp classloader while running down static init path possibilites. When you can use it, it's pretty much always going to be the safer and saner, and more likely correct option. Jetty will set it up to be better, maybe now Solr is getting in on that too:), and who knows what or where else something might use it to be even better than the default goods that come with a built-in per-thread vs per class hierarchy at the least.
So my point was, if not using it, we should point out why. That will help future problem solvers, perhaps educate some code stumblers, and maybe dissuade some future code cut and pasters - at the least. Heck, would have even helped a code review stumbled. I took a look and was immediately weighing the cost of a single comment clarify vs what it would take to actually fully work out and verify myself and man...that's expensive 'potential' to look into right after someone else was eating the effort :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r839995367
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
So `Class.forName(className)` actually uses `ClassLoader.getClassLoader(ClassLoader.getClassLoader(caller))`, which apparently does not work the same as `getClass().getClassLoader()`.
Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r839966279
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -175,6 +175,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
assert null != mdcSnapshot; // prevent compiler warning
MDCLoggingContext.reset();
MDCLoggingContext.setNode(getCores());
+ Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
Review comment:
*NULL_DEREFERENCE:* object returned by `getCores()` could be null and is dereferenced at line 178.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840007011
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
Actually none of this matters. later on `DriverManager.getDriver(connectionUrl)` will check for the driver class on the JDBCStream.class's classLoader. This will fail as well, so no real reason to protect against this for now. We'd have to really get rid of the DriverManager all-together to remove ourselves from this error case.
I'm going to go ahead and just use `Class.forName(className)`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r839957365
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
Should probably add a comment here making it clear there is intention in using this classloader and what that intention is.
I'm a little unclear the intention myself. I belive Class.forName(className) actually calls Class.forName(className, true, this.getClass().getClassLoader()?
Generally you want to avoid both if at all possible. Not only do they potentially behave differently depending on if called from a static context or not, but you pretty much always want to strive for getting the context classloader off the current thread because it will generally at least try and keep the classpath hiearchy sensible - by using a class classloader, there is always high potential that you get instantiated by a classloader higher up the chain than what is used by other application code, and the classes from that code will then be invisible to you.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840072555
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
> Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
More than using any particular ClassLoader, I was looking to suggest a comment to defend, support, and increase maintainability for future devs. This stuff often realised on a lot of pre developed knowledge and new time thinking and tinkering, rough for everyone done the lace to have to face again.
> Unfortunately we can't use SolrResourceLoader here ...
I wasn't suggesting using SolrResourceLoader. Regardless of another ClassLoaders existence, that preference of avoidance exists. That's why it seems useful to leave some bread comes when/if it's used for the future.
Even with no SolrResourceLoader, there is a hierarchy of ClassLoaders in a ServletContainer and I imagine in some other environments as well. This stuff is tricky even out of the box with the default grab bag of System, WebContainer, and Webapp classloaders float around. When you mix in the different and surprising ways that static stuff can end up initialized, using the class ClassLoader is just expert-oriented stuff.
WebApps (and some other frameworks I'm sure) try and lend a hand by setting you up with the per-thread context class loader. You don't want to have to guess or have it swapped around on you about whether you are loading with the container wide classloader or the webapp classloader while running down static init path possibilites. When you can use it, it's pretty much always going to be the safer and saner and more likely correct option. Jetty will set it up to be better, maybe now Solr is getting in on that too:), and who knows what or where else something might use it to be even better than the default goods that come with a built in per-thread vs per class hierarchy at the least.
So my point was, if not using it, we should point out why. That will help future problem solvers, perhaps educate some code stumblers, and maybe disuade some future code cut and pasters - at the least. Heck, would have even helped a code review stumbled. I took a look and was immediately weighing the cost of a single comment clarify vs what it would take to actually fully work out and verify myself and man...that's expensive 'potential' to look into right after someone else was eating the effort :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r839995367
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
So `Class.forName(className)` actually uses `ClassLoader.getClassLoader(Reflection.getCallerClass())`, which apparently does not work the same as `getClass().getClassLoader()`.
Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #783:
URL: https://github.com/apache/solr/pull/783#issuecomment-1084851398
Ok tested this myself, and it passes the "Failed to load JDBC driver" error, but it hits the next error: "Failed to determine JDBC driver from connection url". Will continue testing
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840095238
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -175,6 +175,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
assert null != mdcSnapshot; // prevent compiler warning
MDCLoggingContext.reset();
MDCLoggingContext.setNode(getCores());
+ Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
Review comment:
@sonatype-lift ignore
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #783:
URL: https://github.com/apache/solr/pull/783#issuecomment-1084930252
Ok at this point I get past the loading errors, unsurprisingly. However I do get this new error:
```
java.io.IOException: Failed to execute sqlQuery 'select id from test limit 10' against JDBC connection 'jdbc:calcitesolr:'.
Caused by: Error while executing SQL "select id from test limit 10": Unable to instantiate java compiler
at org.apache.solr.client.solrj.io.stream.JDBCStream.open(JDBCStream.java:333) ~[?:?]
at org.apache.solr.client.solrj.io.stream.ExceptionStream.open(ExceptionStream.java:51) ~[?:?]
....
Caused by: java.sql.SQLException: Error while executing SQL "select id from test limit 10": Unable to instantiate java compiler
at org.apache.calcite.avatica.Helper.createException(Helper.java:56) ~[?:?]
at org.apache.calcite.avatica.Helper.createException(Helper.java:41) ~[?:?]
at org.apache.calcite.avatica.AvaticaStatement.executeInternal(AvaticaStatement.java:163) ~[?:?]
at org.apache.calcite.avatica.AvaticaStatement.executeQuery(AvaticaStatement.java:227) ~[?:?]
at org.apache.solr.client.solrj.io.stream.JDBCStream.open(JDBCStream.java:329) ~[?:?]
... 60 more
Caused by: java.lang.IllegalStateException: Unable to instantiate java compiler
at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.compile(JaninoRelMetadataProvider.java:429) ~[?:?]
at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.load3(JaninoRelMetadataProvider.java:375) ~[?:?]
at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.lambda$static$0(JaninoRelMetadataProvider.java:110) ~[?:?]
.....
Caused by: java.lang.ClassNotFoundException: No implementation of org.codehaus.commons.compiler is on the class path. Typically, you'd have 'janino.jar', or 'commons-compiler-jdk.jar', or both on the classpath.
at org.codehaus.commons.compiler.CompilerFactoryFactory.getDefaultCompilerFactory(CompilerFactoryFactory.java:65) ~[?:?]
at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.compile(JaninoRelMetadataProvider.java:427) ~[?:?]
at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.load3(JaninoRelMetadataProvider.java:375) ~[?:?]
at org.apache.calcite.rel.metadata.JaninoRelMetadataProvider.lambda$static$0(JaninoRelMetadataProvider.java:110) ~[?:?]
```
Will keep debuging
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840072555
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
> Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
More than using any particular ClassLoader, I was looking to suggest a comment to defend, support, and increase maintainability for future devs. This stuff often relies on a lot of pre-developed knowledge and new time thinking and tinkering, rough for anyone down the line to have to face fresh and unaware again.
> Unfortunately we can't use SolrResourceLoader here ...
I wasn't suggesting using SolrResourceLoader. Regardless of another ClassLoaders existence, that preference of avoidance exists. That's why it seems useful to leave some bread crumbs when/if it's used for the future.
Even with no SolrResourceLoader, there is a hierarchy of ClassLoaders in a ServletContainer and I imagine in some other environments as well. This stuff is tricky even out of the box with the default grab bag of System, WebContainer, and Webapp classloaders float around. When you mix in the different and surprising ways that static stuff can end up initialized, using the class ClassLoader is just expert-oriented stuff.
WebApps (and some other frameworks I'm sure) try and lend a hand by setting you up with the per-thread context class loader. You don't want to have to guess or have it swapped around on you about whether you are loading with the container wide classloader or the webapp classloader while running down static init path possibilites. When you can use it, it's pretty much always going to be the safer and saner, and more likely correct option. Jetty will set it up to be better, maybe now Solr is getting in on that too:), and who knows what or where else something might use it to be even better than the default goods that come with a built-in per-thread vs per class hierarchy at the least.
So my point was, if not using it, we should point out why. That will help future problem solvers, perhaps educate some code stumblers, and maybe dissuade some future code cut and pasters - at the least. Heck, would have even helped a code review stumbled. I took a look and was immediately weighing the cost of a single comment clarify vs what it would take to actually fully work out and verify myself and man...that's expensive 'potential' to look into right after someone else was eating the effort :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840072555
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
> Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
More than using any particular ClassLoader, I was looking to suggest a comment to defend, support, and increase maintainability for future devs. This stuff often relies on a lot of pre-developed knowledge and new time thinking and tinkering, rough for anyone down the line to have to face fresh and unaware again.
> Unfortunately we can't use SolrResourceLoader here ...
I wasn't suggesting using SolrResourceLoader. Regardless of another ClassLoaders existence, that preference of avoidance exists. That's why it seems useful to leave some bread crumbs when/if it's used for the future.
Even with no SolrResourceLoader, there is a hierarchy of ClassLoaders in a ServletContainer and I imagine in some other environments as well. This stuff is tricky even out of the box with the default grab bag of System, WebContainer, and Webapp classloaders floating around. When you mix in the different and surprising ways that static stuff can end up initialized, using the class ClassLoader is just expert-oriented stuff.
WebApps (and some other frameworks I'm sure) try and lend a hand by setting you up with the per-thread context class loader. You don't want to have to guess or have it swapped around on you about whether you are loading with the container wide classloader or the WebApp ClassLoader while running down 'static-init path' possibilities. When you can use it, it's pretty much always going to be the safer and saner, and more likely correct option. Jetty will set it up to be better, maybe now Solr is getting in on that too:), and who knows what or where else something might use it to be even better than the default goods that come with a built-in per-thread vs per class hierarchy at the least.
So my point was, if not using it, we should point out why. That will help future problem solvers, perhaps educate some code stumblers, and maybe dissuade some future code cut and pasters - at the least. Heck, would have even helped a code review stumbled. I took a look and was immediately weighing the cost of a single comment clarify vs what it would take to actually fully work out and verify myself and man...that's expensive 'potential' to look into right after someone else was eating the effort :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #783:
URL: https://github.com/apache/solr/pull/783#issuecomment-1085218478
> We need some test coverage on this... But our tests have a different way of bootstrapping than the real thing, so perhaps a docker-test?
Was talking to @madrob about this, who suggested an integration test, which I think is a great idea. Will work on that tomorrow
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] anshumg commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
anshumg commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840011012
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -175,6 +175,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
assert null != mdcSnapshot; // prevent compiler warning
MDCLoggingContext.reset();
MDCLoggingContext.setNode(getCores());
+ Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
Review comment:
should've looked around in the IDE. Thanks for clarifying.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840072555
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
> Unfortunately we can't use SolrResourceLoader here, because streams are in solrj for some reason and SolrResourceLoader is unsurprisingly in core. But agree that now that we better control thread classloaders, we should use that instead.
More than using any particular ClassLoader, I was looking to suggest a comment to defend, support, and increase maintainability for future devs. This stuff often relies on a lot of pre-developed knowledge and new time thinking and tinkering, rough for anyone down the line to have to face fresh and unaware again.
> Unfortunately we can't use SolrResourceLoader here ...
I wasn't suggesting using SolrResourceLoader. Regardless of another ClassLoaders existence, that preference of avoidance exists. That's why it seems useful to leave some bread crumbs when/if it's used for the future.
Even with no SolrResourceLoader, there is a hierarchy of ClassLoaders in a ServletContainer and I imagine in some other environments as well. This stuff is tricky even out of the box with the default grab bag of System, WebContainer, and Webapp classloaders floating around. When you mix in the different and surprising ways that static stuff can end up initialized, using the class ClassLoader is just expert-oriented stuff.
WebApps (and some other frameworks I'm sure) try and lend a hand by setting you up with the per-thread context class loader. You don't want to have to guess or have it swapped around on you about whether you are loading with the container wide classloader or the WebApp ClassLoader while running down 'static-init path' possibilities. When you can use it, it's pretty much always going to be the safer and saner, and more likely correct option. Jetty will set it up to be better, maybe now Solr is getting in on that too:), and who knows what or where else something might use it to be even better than the default goods that come with a built-in per-thread vs per class hierarchy at the least.
So my point was, if not using it, we should point out why. That will help future problem solvers, perhaps educate some code stumblers, and maybe dissuade some future code cut and pasters - at the least. Heck, would have even helped a code review stumbler. I took a look and was immediately weighing the cost of a single comment clarify vs what it would take to actually fully work out and verify myself and man...that's expensive 'potential' to look into right after someone else was eating the effort :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] anshumg commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
anshumg commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r839998163
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -175,6 +175,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
assert null != mdcSnapshot; // prevent compiler warning
MDCLoggingContext.reset();
MDCLoggingContext.setNode(getCores());
+ Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
Review comment:
@HoustonPutman - need to handle 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #783:
URL: https://github.com/apache/solr/pull/783#issuecomment-1085215263
We need some test coverage on this... But our tests have a different way of bootstrapping than the real thing, so perhaps a docker-test?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840008601
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -175,6 +175,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
assert null != mdcSnapshot; // prevent compiler warning
MDCLoggingContext.reset();
MDCLoggingContext.setNode(getCores());
+ Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
Review comment:
This actually can't happen. getCores() will throw an error if cores is null, look at: https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java#L147
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840096485
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/JDBCStream.java
##########
@@ -275,12 +277,10 @@ public void setStreamContext(StreamContext context) {
this.streamContext = context;
}
- /** Opens the JDBCStream */
- public void open() throws IOException {
-
+ protected Driver getDriver() throws IOException {
try {
if (null != driverClassName) {
- Class.forName(driverClassName);
+ Class.forName(driverClassName, true, getClass().getClassLoader());
Review comment:
Good point, I'll add a section explaining the downsides, but the reasoning as to why it is the way it is.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #783: SOLR-16131: Fix class loader for jdbcStream
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #783:
URL: https://github.com/apache/solr/pull/783#discussion_r840096192
##########
File path: solr/core/src/java/org/apache/solr/servlet/SolrDispatchFilter.java
##########
@@ -175,6 +175,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
assert null != mdcSnapshot; // prevent compiler warning
MDCLoggingContext.reset();
MDCLoggingContext.setNode(getCores());
+ Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
Review comment:
I've recorded this as ignored for this pull request. If you change your mind, just comment `@sonatype-lift unignore`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org