Solr
  1. Solr
  2. SOLR-1258

health check file: relative path evaluated against CWD (directory were app was started)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Environment:

      Mac

      Description

      The following element gives admin the ability to enable/disable a solr instance without taking it down

        <admin>
          <defaultQuery>solr</defaultQuery>
      
          <!-- configure a healthcheck file for servers behind a loadbalancer
          -->
          <healthcheck type="file">server-enabled</healthcheck>
        </admin>
      

      There is a problem with where the file 'server-enabled' is placed. Currently it appears that file is relative to the place where the server is stared. This causes trouble to deployment since theoretically the server could be started anywhere. My suggestion is to have the file relative to solr home so that it does not dependent on where the server is started.

      1. SOLR-1258.patch
        27 kB
        Hoss Man
      2. SOLR-1258.patch
        17 kB
        Hoss Man
      3. SOLR-1258.patch
        6 kB
        Hoss Man

        Activity

        Hide
        Jay added a comment -

        This is actually not a big issue as you can always use system property to specify the directory where you put the file.

        Show
        Jay added a comment - This is actually not a big issue as you can always use system property to specify the directory where you put the file.
        Hide
        Hoss Man added a comment -

        I agree that this should be considered a bug, but changing the behavior is non trivial, particularly without confusing existing users on upgrade.

        (it would be easier if we were just testing for the existence of the file, SolrResourceLoader has code to do this in a cascading manner checking the conf dir, instanceDir, and finally CWD) but since Solr creates this file, we need a concrete location. Long term it would make sense to move this healthcheck config out of the legacy <admin> section in solrconfig, and make it an init option on the PingRequestHandler (and move the enable/disable options processing to that handler) if/when we do that we can change the semantics of a relative directory.

        Since Solr has functioned this way for so long, and it has a simple workaround i don't think we should hold back 1.4 waiting on this (but if someone comes up with a nice clean patch, by all means we can move forward)

        Show
        Hoss Man added a comment - I agree that this should be considered a bug, but changing the behavior is non trivial, particularly without confusing existing users on upgrade. (it would be easier if we were just testing for the existence of the file, SolrResourceLoader has code to do this in a cascading manner checking the conf dir, instanceDir, and finally CWD) but since Solr creates this file, we need a concrete location. Long term it would make sense to move this healthcheck config out of the legacy <admin> section in solrconfig, and make it an init option on the PingRequestHandler (and move the enable/disable options processing to that handler) if/when we do that we can change the semantics of a relative directory. Since Solr has functioned this way for so long, and it has a simple workaround i don't think we should hold back 1.4 waiting on this (but if someone comes up with a nice clean patch, by all means we can move forward)
        Hide
        Hoss Man added a comment -

        This issue popped up on the #solr IRC channel today, and looking over the issue and the code i think that for Solr 4.0 we should bite the bullet and...

        • move the configuration of the filename to an init param on the PingRequestHandler itself
          • ie: <str name="healthcheckFile">server-enabled</str>
          • default would be no file (as today) which would mean healthchecking doesn't apply (just the query)
        • change the way the path is resolve to be relative to the dataDir
          • much less kludgy and confusing
          • something solr should deinitely be able to write to
          • way more freindly to multi-core setups
        • add a new example where "/admin/ping" is configured in solrconfig.xml
          • completely remove the example healthcheck from the <admin> block
        • don't bother trying to be backcompatible with the old configuration
          • anyone using that syntax successfully is likely already using an absolute path, and will notice very quickly that it isn't working, and will be able to quickly see from CHANGES.txt that if they move that absolute path to the PingREquestHandler config everything works fine

        Any objections?

        Show
        Hoss Man added a comment - This issue popped up on the #solr IRC channel today, and looking over the issue and the code i think that for Solr 4.0 we should bite the bullet and... move the configuration of the filename to an init param on the PingRequestHandler itself ie: <str name="healthcheckFile">server-enabled</str> default would be no file (as today) which would mean healthchecking doesn't apply (just the query) change the way the path is resolve to be relative to the dataDir much less kludgy and confusing something solr should deinitely be able to write to way more freindly to multi-core setups add a new example where "/admin/ping" is configured in solrconfig.xml completely remove the example healthcheck from the <admin> block don't bother trying to be backcompatible with the old configuration anyone using that syntax successfully is likely already using an absolute path, and will notice very quickly that it isn't working, and will be able to quickly see from CHANGES.txt that if they move that absolute path to the PingREquestHandler config everything works fine Any objections?
        Hide
        Hoss Man added a comment -

        quick stab at what i had in mind.

        a few nocommits relating to docs and better error messages when dirs aren't readable/writable.

        TestPingRequestHandler also still needs fixed (and needs some better tests around how this now works)

        any objections to the general idea?

        Show
        Hoss Man added a comment - quick stab at what i had in mind. a few nocommits relating to docs and better error messages when dirs aren't readable/writable. TestPingRequestHandler also still needs fixed (and needs some better tests around how this now works) any objections to the general idea?
        Hide
        Hoss Man added a comment -

        Fixed & improved the test, improved the error checking involving directory permisions, and added docs.

        Show
        Hoss Man added a comment - Fixed & improved the test, improved the error checking involving directory permisions, and added docs.
        Hide
        Hoss Man added a comment -

        patch just commited, includes cleanup of all the legacy syntax in examples & test configs, as well as the CHANGES.txt note for people upgrading.

        Committed revision 1333598.

        Show
        Hoss Man added a comment - patch just commited, includes cleanup of all the legacy syntax in examples & test configs, as well as the CHANGES.txt note for people upgrading. Committed revision 1333598.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Jay
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 5h
              5h
              Remaining:
              Remaining Estimate - 5h
              5h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development