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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
