Solr
  1. Solr
  2. SOLR-4653

Solr configuration should log inaccessible/ non-existent relative paths <lib dir="...">

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.3, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Currently the dir path is resolved relative to core's instanceDir but if you don't know it nothing happens (no warning, no nothing). This is very frustrating.

        Issue Links

          Activity

          Hide
          Dawid Weiss added a comment -

          Correcting to just logging non-existing relative paths. Absolute paths work, my bad.

          Show
          Dawid Weiss added a comment - Correcting to just logging non-existing relative paths. Absolute paths work, my bad.
          Hide
          Robert Muir added a comment -

          +1 to doing something other than silently not working.

          exception seems ideal to me, but logging would at least be something.

          Show
          Robert Muir added a comment - +1 to doing something other than silently not working. exception seems ideal to me, but logging would at least be something.
          Hide
          Erick Erickson added a comment -

          What! And take the sensitive, appropriate-for-your-aged-grandmother notes out of solrconfig.xml? To whit:

          <!-- If a 'dir' option (with or without a regex) is used and nothing
          is found that matches, it will be ignored
          -->
          <lib dir="/total/crap/dir/ignored" />

          <G>....

          Which is more than a little inconsistent with what follows:

          <!-- an exact 'path' can be used instead of a 'dir' to specify a
          specific jar file. This will cause a serious error to be logged
          if it can't be loaded.
          -->
          <!--
          <lib path="../a-jar-that-does-not-exist.jar" />
          -->

          Why should failing to find a specific jar log a serious error but a directory be ignored?

          Seriously, let's complain if the dir isn't found, at least log a warning if not an error/exception. Fail early, fail often.

          Show
          Erick Erickson added a comment - What! And take the sensitive, appropriate-for-your-aged-grandmother notes out of solrconfig.xml? To whit: <!-- If a 'dir' option (with or without a regex) is used and nothing is found that matches, it will be ignored --> <lib dir="/total/crap/dir/ignored" /> <G>.... Which is more than a little inconsistent with what follows: <!-- an exact 'path' can be used instead of a 'dir' to specify a specific jar file. This will cause a serious error to be logged if it can't be loaded. --> <!-- <lib path="../a-jar-that-does-not-exist.jar" /> --> Why should failing to find a specific jar log a serious error but a directory be ignored? Seriously, let's complain if the dir isn't found, at least log a warning if not an error/exception. Fail early, fail often.
          Hide
          Uwe Schindler added a comment -

          +1, fail early and often

          Show
          Uwe Schindler added a comment - +1, fail early and often
          Hide
          Yonik Seeley added a comment -

          +1 to logging.

          IIRC, it's not a hard fail so that lazy handlers like solr-cell can be supported, but you can still copy "example" to somewhere else and still have all the core stuff work.

          IMO, the larger issue is that we should come up with a better plugin mechanism and not have the stock server referencing outside jars. Or if we have to, do it relative to some kind of $SOLR_INSTALL_DIR or something...

          Show
          Yonik Seeley added a comment - +1 to logging. IIRC, it's not a hard fail so that lazy handlers like solr-cell can be supported, but you can still copy "example" to somewhere else and still have all the core stuff work. IMO, the larger issue is that we should come up with a better plugin mechanism and not have the stock server referencing outside jars. Or if we have to, do it relative to some kind of $SOLR_INSTALL_DIR or something...
          Hide
          Dawid Weiss added a comment -

          I am leaning towards throwing an exception too. A warning should be emitted if the lib doesn't collect any jars (because that's odd too) but if it points at void it looks like an error and in most cases (experienced on my own) what follows is a class-not-found exception because something is referenced that should be loaded and wasn't.

          I agree with Yonik though that it'd be nice to be able to reference based from solr's install dir. But then this may be problematic if people deploy the war in other containers (I know, I know, it's a boo-hoo, but many people do it anyway).

          Show
          Dawid Weiss added a comment - I am leaning towards throwing an exception too. A warning should be emitted if the lib doesn't collect any jars (because that's odd too) but if it points at void it looks like an error and in most cases (experienced on my own) what follows is a class-not-found exception because something is referenced that should be loaded and wasn't. I agree with Yonik though that it'd be nice to be able to reference based from solr's install dir. But then this may be problematic if people deploy the war in other containers (I know, I know, it's a boo-hoo, but many people do it anyway).
          Hide
          Hoss Man added a comment -

          The original intent was:

          • If users want solr to be very strict and fail loudly if your any of your plugin jars can't be found, then you should know what plugin jars you car about and use an explicit "path" option for each of them.
          • for the purposes of the example configs, we wanted something less strict so that (combined with the lazy load option on plugins) the example configs could refer to contrib jars and the bulk of the example configs would still work even if you didn't build any of those contribs, or moved/copied the example configs – if you do either of those things, and then try to use any of those lazy loaded plugins, the lazy loading feature will give you a loud error.

          At this point, i don't have any strong opinions about changing any of this behavior to be a hard failure if the "dir" doesn't exist, or if the regex doesn't match anything – the way the current build system works "ant example" already builds all of the contribs anyway regardless of wether you care about them, so the key concern is really just what would happen when people move/copy the example configs.

          I suspect that if we change this to cause a hard failure the number of users who might be confused/anoyed about why they are getting those errors when they move/copy the example configs (and need to remove those <lib/> directives) will probably be roughly the same as the number of users who (with the existing configs) are confused by the lazy loading errors they get trying to use DIH, or solr-cell, or /browse after they move/copy the configs today and have to go update where the <lib/> directives point at.

          Show
          Hoss Man added a comment - The original intent was: If users want solr to be very strict and fail loudly if your any of your plugin jars can't be found, then you should know what plugin jars you car about and use an explicit "path" option for each of them. for the purposes of the example configs, we wanted something less strict so that (combined with the lazy load option on plugins) the example configs could refer to contrib jars and the bulk of the example configs would still work even if you didn't build any of those contribs, or moved/copied the example configs – if you do either of those things, and then try to use any of those lazy loaded plugins, the lazy loading feature will give you a loud error. At this point, i don't have any strong opinions about changing any of this behavior to be a hard failure if the "dir" doesn't exist, or if the regex doesn't match anything – the way the current build system works "ant example" already builds all of the contribs anyway regardless of wether you care about them, so the key concern is really just what would happen when people move/copy the example configs. I suspect that if we change this to cause a hard failure the number of users who might be confused/anoyed about why they are getting those errors when they move/copy the example configs (and need to remove those <lib/> directives) will probably be roughly the same as the number of users who (with the existing configs) are confused by the lazy loading errors they get trying to use DIH, or solr-cell, or /browse after they move/copy the configs today and have to go update where the <lib/> directives point at.
          Hide
          Yonik Seeley added a comment -

          for the purposes of the example configs, we wanted something less strict so that (combined with the lazy load option on plugins) the example configs could refer to contrib jars [...]

          Right. This is really more a change in behavior than a bug fix, and it seems like we should be comprehensive and also deal with the reasons why it was added in the first place (i.e. we need to figure out what to do with the lazy loaded plugins like solr-cell). Docs like the README under example, and http://wiki.apache.org/solr/ExtractingRequestHandler would also need to change.

          Changing to a hard fail has back compat issues too. We have often recommended copying the "example" directory as a starting point, and most of those will be pointing to directores that don't exist.

          On balance it feels like not failing hard (i.e. keeping the current behavior, but adding logging) is the best middle ground right now. If one does get a class-not-found exception, we'll have the better logging to help track it down.

          Show
          Yonik Seeley added a comment - for the purposes of the example configs, we wanted something less strict so that (combined with the lazy load option on plugins) the example configs could refer to contrib jars [...] Right. This is really more a change in behavior than a bug fix, and it seems like we should be comprehensive and also deal with the reasons why it was added in the first place (i.e. we need to figure out what to do with the lazy loaded plugins like solr-cell). Docs like the README under example, and http://wiki.apache.org/solr/ExtractingRequestHandler would also need to change. Changing to a hard fail has back compat issues too. We have often recommended copying the "example" directory as a starting point, and most of those will be pointing to directores that don't exist. On balance it feels like not failing hard (i.e. keeping the current behavior, but adding logging) is the best middle ground right now. If one does get a class-not-found exception, we'll have the better logging to help track it down.
          Hide
          Dawid Weiss added a comment -

          Thanks for clarification, Hoss. What you say makes sense but the common practice seems to be that:

          • people modify the example's configuration files and overwrite them just because all the paths are relative from those directories and pointing at contribs,
          • when you want to create an isolate configuration and pass it to somebody to, say, run with -Dsolr.solr.home it's hard because you need to know absolute installation paths or make the user install that configuration in an exact folder under solr's unpacked structure. If they change anything – boom, things don't work and nobody knows what's going on.

          I think the middle ground here is to at least log a warning when a fileset is empty (and provide the basedir against which it was resolved). This looks suspicious and provides enough information to recover from the problem.

          Show
          Dawid Weiss added a comment - Thanks for clarification, Hoss. What you say makes sense but the common practice seems to be that: people modify the example's configuration files and overwrite them just because all the paths are relative from those directories and pointing at contribs, when you want to create an isolate configuration and pass it to somebody to, say, run with -Dsolr.solr.home it's hard because you need to know absolute installation paths or make the user install that configuration in an exact folder under solr's unpacked structure. If they change anything – boom, things don't work and nobody knows what's going on. I think the middle ground here is to at least log a warning when a fileset is empty (and provide the basedir against which it was resolved). This looks suspicious and provides enough information to recover from the problem.
          Hide
          Dawid Weiss added a comment -

          This is my take at this having considered a few other alternatives. It logs this at startup currently:

          Can't find (or read) directory to add to classloader: /total/crap/dir/ignored (resolved as: C:\Work\lucene-solr-svn\trunk\solr\example\solr\collection1\total\crap\dir\ignored).
          

          And indeed the example config contains:

            <!-- If a 'dir' option (with or without a regex) is used and nothing
                 is found that matches, it will be ignored
              -->
            <lib dir="/total/crap/dir/ignored" /> 
          

          Perhaps the comment should be changed/ made more polite too.

          Show
          Dawid Weiss added a comment - This is my take at this having considered a few other alternatives. It logs this at startup currently: Can't find (or read) directory to add to classloader: /total/crap/dir/ignored (resolved as: C:\Work\lucene-solr-svn\trunk\solr\example\solr\collection1\total\crap\dir\ignored). And indeed the example config contains: <!-- If a 'dir' option (with or without a regex) is used and nothing is found that matches, it will be ignored --> <lib dir= "/total/crap/dir/ignored" /> Perhaps the comment should be changed/ made more polite too.
          Hide
          Dawid Weiss added a comment -

          If no objections I'll commit this in a few hours.

          Show
          Dawid Weiss added a comment - If no objections I'll commit this in a few hours.
          Hide
          Uwe Schindler added a comment - - edited

          Just one additional idea: There are comments about the regex in the current code and complexity for users. How about using ANT-style filesets for specifying the lib dirs? We dont need ANT for that, just plexus-utils-1.1.jar from Maven, which contains DirectoryScanner that is able to parse things like **/*.jar. I use that in the fobidden APIs CLI and Mojo classes: https://code.google.com/p/forbidden-apis/source/browse/trunk/src/main/java/de/thetaphi/forbiddenapis/CliMain.java#257

          Otherwise +1 to commit!

          Show
          Uwe Schindler added a comment - - edited Just one additional idea: There are comments about the regex in the current code and complexity for users. How about using ANT-style filesets for specifying the lib dirs? We dont need ANT for that, just plexus-utils-1.1.jar from Maven, which contains DirectoryScanner that is able to parse things like **/*.jar . I use that in the fobidden APIs CLI and Mojo classes: https://code.google.com/p/forbidden-apis/source/browse/trunk/src/main/java/de/thetaphi/forbiddenapis/CliMain.java#257 Otherwise +1 to commit!
          Hide
          Erick Erickson added a comment -

          +1 to making the message more polite. The rest of the patch too!

          Show
          Erick Erickson added a comment - +1 to making the message more polite. The rest of the patch too!
          Hide
          Dawid Weiss added a comment -

          I saw the comment about glob patterns, Uwe. I just won't have the time to do it thoroughly (with proper testing etc.) so I'd rather file another issue for it. If somebody can get there sooner, fine. If not, I'll fix it in my spare cycles at some point.

          Show
          Dawid Weiss added a comment - I saw the comment about glob patterns, Uwe. I just won't have the time to do it thoroughly (with proper testing etc.) so I'd rather file another issue for it. If somebody can get there sooner, fine. If not, I'll fix it in my spare cycles at some point.
          Hide
          Jan Høydahl added a comment -

          Dawid Weiss logging proposal is ok.

          However, a better way would be to fail hard (can be made back-compat by checking luceneMatchVersion and log only for <4.3). In that case how we include contrib libs should change somehow. Perhaps something like this:

          <lib>${solr.contrib.root}/extraction/lib</lib>
          

          If user supplies -Dsolr.contrib.root=/path/to/contrib then stuff will work. If nothing is passed by opts we could default to something that would work for the shipped example config.

          Also see my proposal in SOLR-4495. For most of my multi-core deploys I don't bother adding lib entries to all cores, but use a single sharedLib=${solr.solr.home}/lib in solr.xml. Allowing multiple sharedLib dirs in solr.xml would allow for more flexible config.

          Show
          Jan Høydahl added a comment - Dawid Weiss logging proposal is ok. However, a better way would be to fail hard (can be made back-compat by checking luceneMatchVersion and log only for <4.3). In that case how we include contrib libs should change somehow. Perhaps something like this: <lib>${solr.contrib.root}/extraction/lib</lib> If user supplies -Dsolr.contrib.root=/path/to/contrib then stuff will work. If nothing is passed by opts we could default to something that would work for the shipped example config. Also see my proposal in SOLR-4495 . For most of my multi-core deploys I don't bother adding lib entries to all cores, but use a single sharedLib=${solr.solr.home}/lib in solr.xml. Allowing multiple sharedLib dirs in solr.xml would allow for more flexible config.
          Hide
          Dawid Weiss added a comment -

          This (the variable substitution) should also work as far as I'm concerned because variables are substituted globally in all text nodes. That's my understanding of the code, didn't check on a live instance. So if you passed $

          {solr.contrib.root}

          and it doesn't point anywhere you'd get a warning in the logs.

          Since some folks expressed their concern about failing hard (and it was actually documented that it won't fail hard) I think it makes sense to preserve this behavior, at least in the same Solr line (4.x). Perhaps another issue changing this behavior should be filed for 5.x.

          Show
          Dawid Weiss added a comment - This (the variable substitution) should also work as far as I'm concerned because variables are substituted globally in all text nodes. That's my understanding of the code, didn't check on a live instance. So if you passed $ {solr.contrib.root} and it doesn't point anywhere you'd get a warning in the logs. Since some folks expressed their concern about failing hard (and it was actually documented that it won't fail hard) I think it makes sense to preserve this behavior, at least in the same Solr line (4.x). Perhaps another issue changing this behavior should be filed for 5.x.
          Hide
          Uwe Schindler added a comment -

          Closed after release.

          Show
          Uwe Schindler added a comment - Closed after release.

            People

            • Assignee:
              Dawid Weiss
              Reporter:
              Dawid Weiss
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development