Solr
  1. Solr
  2. SOLR-5459

Try loading a temporary core when saving a file in the admin UI config editing mode

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Implemented
    • Affects Version/s: None
    • Fix Version/s: 4.7, 5.0
    • Component/s: None
    • Labels:
      None

      Description

      SOLR-5287 adds the ability to shoot yourself in the foot by changing core config files such that the core can't load, leading to having to go out into the filesystem and manually edit the files until you can reload the core. This is clumsy at best.

      Mark Miller suggested creating a temporary core and trying to reload it before saving the "real" changes, which gives us an approach to the problem that seems relatively easy to implement.

      1. SOLR-5459.patch
        4 kB
        Erick Erickson
      2. SOLR-5459.patch
        11 kB
        Erick Erickson

        Issue Links

          Activity

          Hide
          Erick Erickson added a comment -

          Here's a quick hack at a patch that does this. It makes me quite nervous. It looks remarkably like some of our test code, creating a temporary directory, copying the conf directory over, and then trying to load it. If you navigate away from the edit page, it loses any changes you've made.

          It also takes a while longer, and will take even longer from ZooKeeper. Do we really want to bring the conf directory down from ZK, test it, and throw it away? Or is there a better way?

          It has to clean up after itself by doing a FileUtils.deleteDirectory().

          I've walked through it by hand and modified one of the local filesystem tests to try it and it appears to work.

          Stefan Matheis (steffkes) The response returned will now have an error in it, but that's not displayed. This is a marker, I'm not sure whether we want to go forward with this or not. And if an error is returned the progress indicator on the save button never stops.

          Mark Miller is this anything like what you had in mind?

          NOTE: I need to clean it up, but another instance of throwing something up that's not ready to see what people think.

          Still to do is the ZooKeeper version.

          Show
          Erick Erickson added a comment - Here's a quick hack at a patch that does this. It makes me quite nervous. It looks remarkably like some of our test code, creating a temporary directory, copying the conf directory over, and then trying to load it. If you navigate away from the edit page, it loses any changes you've made. It also takes a while longer, and will take even longer from ZooKeeper. Do we really want to bring the conf directory down from ZK, test it, and throw it away? Or is there a better way? It has to clean up after itself by doing a FileUtils.deleteDirectory(). I've walked through it by hand and modified one of the local filesystem tests to try it and it appears to work. Stefan Matheis (steffkes) The response returned will now have an error in it, but that's not displayed. This is a marker, I'm not sure whether we want to go forward with this or not. And if an error is returned the progress indicator on the save button never stops. Mark Miller is this anything like what you had in mind? NOTE: I need to clean it up, but another instance of throwing something up that's not ready to see what people think. Still to do is the ZooKeeper version.
          Hide
          Stefan Matheis (steffkes) added a comment -

          That's a good hint Erick, need to catch the failure case - but should be done w/ or w/o this issue (:

          Show
          Stefan Matheis (steffkes) added a comment - That's a good hint Erick, need to catch the failure case - but should be done w/ or w/o this issue (:
          Hide
          Erick Erickson added a comment -

          What do people think? Particularly Mark Miller and Stefan Matheis (steffkes)? Loading things from Zk for every file save will be slow. OTOH, having this will prevent people from shooting themselves in the foot. I see two ways to go about this

          1> every file save goes through the verification process. This is kind of expensive for, say, changing the synonyms file. Or all the language files. Or....
          2> we (well, Stefan) provides a "test config" button that does this. This would just be setting the "op" param to test, as "op=test". The user could save as many different files as they wanted and then test once.

          All in all, I like <2> better. It should still allow someone to recover from messing things up provided they test before reloading the core. And it allows people to bypass the whole verification process if, for some reason, creating the temp core and trying to load it is broken due to my coding skill deficiencies...

          Show
          Erick Erickson added a comment - What do people think? Particularly Mark Miller and Stefan Matheis (steffkes) ? Loading things from Zk for every file save will be slow. OTOH, having this will prevent people from shooting themselves in the foot. I see two ways to go about this 1> every file save goes through the verification process. This is kind of expensive for, say, changing the synonyms file. Or all the language files. Or.... 2> we (well, Stefan) provides a "test config" button that does this. This would just be setting the "op" param to test, as "op=test". The user could save as many different files as they wanted and then test once. All in all, I like <2> better. It should still allow someone to recover from messing things up provided they test before reloading the core. And it allows people to bypass the whole verification process if, for some reason, creating the temp core and trying to load it is broken due to my coding skill deficiencies...
          Hide
          Mark Miller added a comment -

          I think at least initially, 2 is preferable. We need a solid and fast option if we are going to force sanity checking I think. 2 is best for now and let's us brute force the issue for now.

          Show
          Mark Miller added a comment - I think at least initially, 2 is preferable. We need a solid and fast option if we are going to force sanity checking I think. 2 is best for now and let's us brute force the issue for now.
          Hide
          Erick Erickson added a comment -

          Stefan Matheis (steffkes) Can I ask you to provide a "test" button on the UI? It would go to the ShowFileRequestHandler with the stream set to the current file and, say, op="test"?

          Thinking it over, one advantage of this is that if, for some reason, creating all the temp stuff doesn't work (permissions, whatever) they can still edit through the UI. It puts testing under explicit control.

          I should check in the relevant code tonight.

          Show
          Erick Erickson added a comment - Stefan Matheis (steffkes) Can I ask you to provide a "test" button on the UI? It would go to the ShowFileRequestHandler with the stream set to the current file and, say, op="test"? Thinking it over, one advantage of this is that if, for some reason, creating all the temp stuff doesn't work (permissions, whatever) they can still edit through the UI. It puts testing under explicit control. I should check in the relevant code tonight.
          Hide
          Stefan Matheis (steffkes) added a comment -

          sure thing, i'll try to bring that in with SOLR-5457 which is still on the list

          Show
          Stefan Matheis (steffkes) added a comment - sure thing, i'll try to bring that in with SOLR-5457 which is still on the list
          Hide
          Erick Erickson added a comment -

          Final patch with tests changed to accommodate a "test" option.

          Show
          Erick Erickson added a comment - Final patch with tests changed to accommodate a "test" option.
          Hide
          ASF subversion and git services added a comment -

          Commit 1543660 from Erick Erickson in branch 'dev/trunk'
          [ https://svn.apache.org/r1543660 ]

          SOLR-5459: Try loading a temporary core when saving a file in the admin UI config editing mode

          Show
          ASF subversion and git services added a comment - Commit 1543660 from Erick Erickson in branch 'dev/trunk' [ https://svn.apache.org/r1543660 ] SOLR-5459 : Try loading a temporary core when saving a file in the admin UI config editing mode
          Hide
          ASF subversion and git services added a comment -

          Commit 1543685 from Erick Erickson in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1543685 ]

          SOLR-5459: Try loading a temporary core when saving a file in the admin UI config editing mode

          Show
          ASF subversion and git services added a comment - Commit 1543685 from Erick Erickson in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1543685 ] SOLR-5459 : Try loading a temporary core when saving a file in the admin UI config editing mode
          Hide
          Erick Erickson added a comment -

          Stefan Matheis (steffkes) Ready for the UI work with op=test.

          Show
          Erick Erickson added a comment - Stefan Matheis (steffkes) Ready for the UI work with op=test.

            People

            • Assignee:
              Erick Erickson
              Reporter:
              Erick Erickson
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development