Solr
  1. Solr
  2. SOLR-8246

create_core command gives confusing messages if broken config is given, fixed, given again

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 5.3.1
    • Fix Version/s: 5.4, 6.0
    • Component/s: scripts and tools
    • Labels:

      Description

      If the config used to create a core, the command complains and aborts:

      Example:

      $ bin/solr create_core -c minicore -d ../miniconf/
      
      Creating new core 'minicore' using command:
      http://localhost:8983/solr/admin/cores?action=CREATE&name=minicore&instanceDir=minicore
      
      ERROR: Error CREATEing SolrCore 'minicore': Unable to create core [minicore] Caused by: The element type "dynamicField" must be terminated by the matching end-tag "</dynamicField>".
      

      However, after fixing the problem, trying to re-run the command gives the same error, causing all sorts of confusion.

      What actually happened is that config has been copied to the destination directory and then when the command is run the second time, it does not recopy fresh - and fixed - files, but just reuses the previously copied - broken - ones.

      The workaround is to manually delete created destination directory and rerun the command. But it is far from obvious.

      The fix would be either undoing the copy - a delete. The minimal fix would be a log statement that the directory was copied (which would help example logs too). And perhaps an extra message when the create command failed that files were copied and may need to be manually deleted.

      1. SOLR-8246.patch
        1 kB
        Jason Gerlowski
      2. SOLR-8246.patch
        2 kB
        Jason Gerlowski

        Activity

        Hide
        Jason Gerlowski added a comment -

        I'll take a stab at fixing this, unless you've already started working on it Alex?

        Show
        Jason Gerlowski added a comment - I'll take a stab at fixing this, unless you've already started working on it Alex?
        Hide
        Alexandre Rafalovitch added a comment -

        I have not. Feel free to go ahead.

        Show
        Alexandre Rafalovitch added a comment - I have not. Feel free to go ahead.
        Hide
        Jason Gerlowski added a comment -

        This patch changes SolrCLI.java to catch SolrServerException when trying (and failing) to create a core. When the exception is caught, we delete the copy of the config (in configInstanceDir, and rethrow the error so it can be parsed and displayed elsewhere.

        I also massage one of the log messages to make it clear that the config is being copied, as Alex suggested.

        Show
        Jason Gerlowski added a comment - This patch changes SolrCLI.java to catch SolrServerException when trying (and failing) to create a core. When the exception is caught, we delete the copy of the config (in configInstanceDir , and rethrow the error so it can be parsed and displayed elsewhere. I also massage one of the log messages to make it clear that the config is being copied, as Alex suggested.
        Hide
        Jason Gerlowski added a comment -

        Can someone please take a look at this patch when they get a few minutes? If it's not wanted, or is too low priority to look at right now, that's fine with me too. It is a pretty minor in the scheme of things. I just wanted to make sure it doesn't get lost accidentally.

        Show
        Jason Gerlowski added a comment - Can someone please take a look at this patch when they get a few minutes? If it's not wanted, or is too low priority to look at right now, that's fine with me too. It is a pretty minor in the scheme of things. I just wanted to make sure it doesn't get lost accidentally.
        Hide
        Shai Erera added a comment -

        Looks good. The only comment that I have is the addition of Throwables dependency. The method runImpl already throws Exception, so can we just catch SolrServerException and rethrow it?

        Show
        Shai Erera added a comment - Looks good. The only comment that I have is the addition of Throwables dependency. The method runImpl already throws Exception , so can we just catch SolrServerException and rethrow it?
        Hide
        Jason Gerlowski added a comment -

        Sure, Throwables doesn't add a ton. I'll take it out.

        Show
        Jason Gerlowski added a comment - Sure, Throwables doesn't add a ton. I'll take it out.
        Hide
        Shai Erera added a comment -

        Since you haven't upload a new patch yet, Is there any reason not to catch Exception, instead of SolrServerException? Don't we want to delete the directory in any case?

        Show
        Shai Erera added a comment - Since you haven't upload a new patch yet, Is there any reason not to catch Exception, instead of SolrServerException? Don't we want to delete the directory in any case?
        Hide
        Jason Gerlowski added a comment -

        Sure.

        I hadn't initially done that because I was trying to keep the effect of my change as small as possible. But in hindsight it's probably safe to delete the copied config regardless. (Recopying is cheap, and it never hurts to clean up after yourself.) Will change.

        Show
        Jason Gerlowski added a comment - Sure. I hadn't initially done that because I was trying to keep the effect of my change as small as possible. But in hindsight it's probably safe to delete the copied config regardless. (Recopying is cheap, and it never hurts to clean up after yourself.) Will change.
        Hide
        Jason Gerlowski added a comment -

        Attached patch makes the changes Shai recommended in his two comments above.

        Show
        Jason Gerlowski added a comment - Attached patch makes the changes Shai recommended in his two comments above.
        Hide
        Alexandre Rafalovitch added a comment -

        One common use case to keep in mind is people rerunning /bin/solr start -e examplename multiple times. We need to make sure that nothing weird happens in that case.

        Show
        Alexandre Rafalovitch added a comment - One common use case to keep in mind is people rerunning /bin/solr start -e examplename multiple times. We need to make sure that nothing weird happens in that case.
        Hide
        Jason Gerlowski added a comment -

        Just spent half an hour or so messing around with different interleavings of bin/solr start -e techproducts|cloud|etc, bin/solr stop -all, and mangling/fixing solr-config files. I didn't find any edge cases that produced unexpected results. Though if you had something in particular you wanted to try I'd definitely appreciate the double-check.

        Show
        Jason Gerlowski added a comment - Just spent half an hour or so messing around with different interleavings of bin/solr start -e techproducts|cloud|etc , bin/solr stop -all , and mangling/fixing solr-config files. I didn't find any edge cases that produced unexpected results. Though if you had something in particular you wanted to try I'd definitely appreciate the double-check.
        Hide
        ASF subversion and git services added a comment -

        Commit 1714764 from Shai Erera in branch 'dev/trunk'
        [ https://svn.apache.org/r1714764 ]

        SOLR-8246: fix SolrCLI to delete config directory if creating a core failed

        Show
        ASF subversion and git services added a comment - Commit 1714764 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1714764 ] SOLR-8246 : fix SolrCLI to delete config directory if creating a core failed
        Hide
        ASF subversion and git services added a comment -

        Commit 1714765 from Shai Erera in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1714765 ]

        SOLR-8246: fix SolrCLI to delete config directory if creating a core failed

        Show
        ASF subversion and git services added a comment - Commit 1714765 from Shai Erera in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1714765 ] SOLR-8246 : fix SolrCLI to delete config directory if creating a core failed
        Hide
        Shai Erera added a comment -

        Committed to trunk and 5x. I also added a CHANGES entry. Thanks Jason Gerlowski!

        Show
        Shai Erera added a comment - Committed to trunk and 5x. I also added a CHANGES entry. Thanks Jason Gerlowski !

          People

          • Assignee:
            Shai Erera
            Reporter:
            Alexandre Rafalovitch
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development