Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      When using simple or native lockType and the application server is not shutdown properly (kill -9), you don't notice problems until someone tries to add or delete a document. In fact, you get errors every time Solr opens a new IndexWriter on the "locked" index. I'm aware of the unlockOnStartup option, but I'd prefer to know and act properly when there's a lock, and I think it would be better to know on startup, since Solr is not going to work properly.

      1. SOLR-3156.patch
        8 kB
        Luca Cavanna
      2. SOLR-3156.patch
        20 kB
        Luca Cavanna

        Activity

        Hide
        Luca Cavanna added a comment -

        I'd have a patch almost ready, but I'd like to hear your thoughts first of all.

        I'd add a new check in the SolrCore#initIndex method. Would it be ok to try opening a SolrIndexWriter and close it immediately? That way if there's a lock and simple or native lockType, you'll immediately see an exception and Solr won't start, instead of notice the problem only when the first document is submitted. Should we add an option to the config for this check and make it disabled by default?

        Could you let me know what you think guys?

        Show
        Luca Cavanna added a comment - I'd have a patch almost ready, but I'd like to hear your thoughts first of all. I'd add a new check in the SolrCore#initIndex method. Would it be ok to try opening a SolrIndexWriter and close it immediately? That way if there's a lock and simple or native lockType, you'll immediately see an exception and Solr won't start, instead of notice the problem only when the first document is submitted. Should we add an option to the config for this check and make it disabled by default? Could you let me know what you think guys?
        Hide
        Martijn van Groningen added a comment - - edited

        I also prefer to know that the index is locked at startup than when the first document is added!
        Opening and then closing a SolrIndexWriter seems like the most easy way to check this in the SolrCore#initIndex method.

        Show
        Martijn van Groningen added a comment - - edited I also prefer to know that the index is locked at startup than when the first document is added! Opening and then closing a SolrIndexWriter seems like the most easy way to check this in the SolrCore#initIndex method.
        Hide
        Luca Cavanna added a comment -

        Attached the first draft patch with a new test class. I've changed my mind a little bit and put the check together with the unlockOnStartup code. There we already know if the index is locked (IndexWriter#isLocked). If unlockOnStartup is enabled we remove the lock, otherwise we throw a LockObtainFailedException, the same exception we would have seen at the first document submission. What do you think guys? is this the right direction?

        Show
        Luca Cavanna added a comment - Attached the first draft patch with a new test class. I've changed my mind a little bit and put the check together with the unlockOnStartup code. There we already know if the index is locked (IndexWriter#isLocked). If unlockOnStartup is enabled we remove the lock, otherwise we throw a LockObtainFailedException, the same exception we would have seen at the first document submission. What do you think guys? is this the right direction?
        Hide
        Hoss Man added a comment -

        Luca: the change to SolrCore looks good to me ... the one thing i might suggest is adding an ERROR log just before you throw the exception (i'm in the "log early" team)

        The test looks awesome, but PLEASE trim those solrconfig files down so that they only contain the 5-6 minimum lines they need inorder for the test to be meaningful ... we have far too many big bloated test configs already, the goal is to stop adding new ones and make sure each test config has a specific and easy to see purpose.

        Show
        Hoss Man added a comment - Luca: the change to SolrCore looks good to me ... the one thing i might suggest is adding an ERROR log just before you throw the exception (i'm in the "log early" team) The test looks awesome, but PLEASE trim those solrconfig files down so that they only contain the 5-6 minimum lines they need inorder for the test to be meaningful ... we have far too many big bloated test configs already, the goal is to stop adding new ones and make sure each test config has a specific and easy to see purpose.
        Hide
        Luca Cavanna added a comment -

        Hi Hoss,
        thanks for your feedback.

        the one thing i might suggest is adding an ERROR log just before you throw the exception (i'm in the "log early" team)

        I've added the log before throwing exception.

        trim those solrconfig files down so that they only contain the 5-6 minimum lines they need inorder for the test to be meaningful

        I completely agree, I already did a little, but I just did it (a lot) more. You'll like it now.

        Show
        Luca Cavanna added a comment - Hi Hoss, thanks for your feedback. the one thing i might suggest is adding an ERROR log just before you throw the exception (i'm in the "log early" team) I've added the log before throwing exception. trim those solrconfig files down so that they only contain the 5-6 minimum lines they need inorder for the test to be meaningful I completely agree, I already did a little, but I just did it (a lot) more. You'll like it now.
        Hide
        Luca Cavanna added a comment -

        Anything else I can do to have this committed? Does everybody agree on this change?

        Show
        Luca Cavanna added a comment - Anything else I can do to have this committed? Does everybody agree on this change?
        Hide
        Martijn van Groningen added a comment -

        Looks good. I will commit this soon.

        Show
        Martijn van Groningen added a comment - Looks good. I will commit this soon.
        Hide
        Martijn van Groningen added a comment -

        Committed to trunk and branch 3x.

        Show
        Martijn van Groningen added a comment - Committed to trunk and branch 3x.
        Hide
        Luca Cavanna added a comment -

        Thanks Martijn!

        Show
        Luca Cavanna added a comment - Thanks Martijn!

          People

          • Assignee:
            Martijn van Groningen
            Reporter:
            Luca Cavanna
          • Votes:
            2 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development