Solr
  1. Solr
  2. SOLR-7307

Make it easier to create an EmbeddedSolrServer

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      At the moment, if you want to create an EmbeddedSolrServer you have to instantiate a CoreContainer and then pass it in to the ESS constructor, which involves a fair amount of ceremony. You should be able to just pass a path to a solr home directory, or a NodeConfig object.

      1. SOLR-7307.patch
        20 kB
        Alan Woodward
      2. SOLR-7307.patch
        20 kB
        Alan Woodward
      3. SOLR-7307.patch
        13 kB
        Erick Erickson
      4. SOLR-7307.patch
        13 kB
        Alan Woodward

        Activity

        Hide
        Alan Woodward added a comment -

        Patch, adding two new constructors:

        • EmbeddedSolrServer(Path)
        • EmbeddedSolrServer(NodeConfig)

        This also deprecates the EmbeddedSolrServer(SolrCore) constructor, as you need to have a CoreContainer anyway, and rejigs the request() method so that you can fire CoreAdmin requests at it before there are any cores.

        Show
        Alan Woodward added a comment - Patch, adding two new constructors: EmbeddedSolrServer(Path) EmbeddedSolrServer(NodeConfig) This also deprecates the EmbeddedSolrServer(SolrCore) constructor, as you need to have a CoreContainer anyway, and rejigs the request() method so that you can fire CoreAdmin requests at it before there are any cores.
        Hide
        Erick Erickson added a comment -

        Same patch with formatting corrections, taking a page from Mark's book .

        Show
        Erick Erickson added a comment - Same patch with formatting corrections, taking a page from Mark's book .
        Hide
        Alan Woodward added a comment -

        Fixed a couple of test failures to do with exception reporting. I think this is ready?

        Show
        Alan Woodward added a comment - Fixed a couple of test failures to do with exception reporting. I think this is ready?
        Hide
        Timothy Potter added a comment -

        Be good to pass a path to a config directory in ZK too ... I do something similar in the Solr Spark project:
        https://github.com/LucidWorks/spark-solr/blob/master/src/main/java/com/lucidworks/spark/util/EmbeddedSolrServerFactory.java

        Show
        Timothy Potter added a comment - Be good to pass a path to a config directory in ZK too ... I do something similar in the Solr Spark project: https://github.com/LucidWorks/spark-solr/blob/master/src/main/java/com/lucidworks/spark/util/EmbeddedSolrServerFactory.java
        Hide
        Mike Drob added a comment - - edited
        +  /**
        +   * Create an EmbeddedSolrServer wrapping a particular SolrCore
        +   *
        +   * @deprecated
        +   */
        +  @Deprecated
        +  public EmbeddedSolrServer(SolrCore core) {
        

        Nit: Point users at the new preferred constructor as part of the javadoc.

        +        if (resp.getException() != null) {
        +          if (resp.getException() instanceof SolrException) {
        +            throw resp.getException();
        +          }
        +          throw new SolrServerException(resp.getException());
        +        }
        

        This logic appears multiple times. Extract to a method?

        +      req = _parser.buildRequestFrom(core, params, request.getContentStreams());
        +      req.getContext().put("path", path);
        

        Maybe we use another try-with-resources here?

        Show
        Mike Drob added a comment - - edited + /** + * Create an EmbeddedSolrServer wrapping a particular SolrCore + * + * @deprecated + */ + @Deprecated + public EmbeddedSolrServer(SolrCore core) { Nit: Point users at the new preferred constructor as part of the javadoc. + if (resp.getException() != null ) { + if (resp.getException() instanceof SolrException) { + throw resp.getException(); + } + throw new SolrServerException(resp.getException()); + } This logic appears multiple times. Extract to a method? + req = _parser.buildRequestFrom(core, params, request.getContentStreams()); + req.getContext().put( "path" , path); Maybe we use another try-with-resources here?
        Hide
        Alan Woodward added a comment -

        I don't think we should be automatically creating cores here? The idea is that either you can point the ESS at an already-existing installation, or you can create cores via CoreAdmin requests (as I do in the tests in this patch). I'm not sure how well ESS would play with ZK and a SolrCloud setup anyway, as it can't respond to requests over HTTP.

        Show
        Alan Woodward added a comment - I don't think we should be automatically creating cores here? The idea is that either you can point the ESS at an already-existing installation, or you can create cores via CoreAdmin requests (as I do in the tests in this patch). I'm not sure how well ESS would play with ZK and a SolrCloud setup anyway, as it can't respond to requests over HTTP.
        Hide
        Alan Woodward added a comment -

        Thanks for the review, Mike! Here's an updated patch

        Unfortunately, SolrQueryRequest doesn't implement Closeable, so we can't use try-with-resources. One for another issue, maybe.

        I decided to leave that constructor as undeprecated in the end.

        Show
        Alan Woodward added a comment - Thanks for the review, Mike! Here's an updated patch Unfortunately, SolrQueryRequest doesn't implement Closeable, so we can't use try-with-resources. One for another issue, maybe. I decided to leave that constructor as undeprecated in the end.
        Hide
        Timothy Potter added a comment -

        My suggestion had nothing to do with SolrCloud. ZK is another common / easy location to pull configs from for creating ESS

        Show
        Timothy Potter added a comment - My suggestion had nothing to do with SolrCloud. ZK is another common / easy location to pull configs from for creating ESS
        Hide
        Alan Woodward added a comment -

        Oh, I see. I think that's probably for another issue, though, and maybe better done via a new ConfigSetService impl?

        Show
        Alan Woodward added a comment - Oh, I see. I think that's probably for another issue, though, and maybe better done via a new ConfigSetService impl?
        Hide
        Timothy Potter added a comment -

        Ok good idea ... tackle that in another ticket.

        Show
        Timothy Potter added a comment - Ok good idea ... tackle that in another ticket.
        Hide
        Mike Drob added a comment -
        -        ("Can't delete 'NewField1' because it's referred to by at least one copy field directive"));
        +        ("Can't delete field 'NewField1' because it's referred to by at least one copy field directive"));
        

        This looks unrelated? I get a test failure from it ant test -Dtestcase=TestBulkSchemaAPI -Dtests.method=testDeleteAndReplace -Dtests.seed=7F43B450F869BCF -Dtests.slow=true -Dtests.locale=sr_RS -Dtests.timezone=America/Dawson_Creek -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1

        Show
        Mike Drob added a comment - - ( "Can't delete 'NewField1' because it's referred to by at least one copy field directive" )); + ( "Can't delete field 'NewField1' because it's referred to by at least one copy field directive" )); This looks unrelated? I get a test failure from it ant test -Dtestcase=TestBulkSchemaAPI -Dtests.method=testDeleteAndReplace -Dtests.seed=7F43B450F869BCF -Dtests.slow=true -Dtests.locale=sr_RS -Dtests.timezone=America/Dawson_Creek -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1
        Hide
        Alan Woodward added a comment -

        It's an unrelated test fix that's already been committed - see http://svn.apache.org/r1669173

        Show
        Alan Woodward added a comment - It's an unrelated test fix that's already been committed - see http://svn.apache.org/r1669173
        Hide
        ASF subversion and git services added a comment -

        Commit 1669305 from Alan Woodward in branch 'dev/trunk'
        [ https://svn.apache.org/r1669305 ]

        SOLR-7307: Add constructors to EmbeddedSolrServer taking Path or NodeConfig

        Show
        ASF subversion and git services added a comment - Commit 1669305 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1669305 ] SOLR-7307 : Add constructors to EmbeddedSolrServer taking Path or NodeConfig
        Hide
        ASF subversion and git services added a comment -

        Commit 1669308 from Alan Woodward in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1669308 ]

        SOLR-7307: Add constructors to EmbeddedSolrServer taking Path or NodeConfig

        Show
        ASF subversion and git services added a comment - Commit 1669308 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1669308 ] SOLR-7307 : Add constructors to EmbeddedSolrServer taking Path or NodeConfig
        Hide
        Alan Woodward added a comment -

        Thanks all.

        Show
        Alan Woodward added a comment - Thanks all.
        Hide
        Shai Erera added a comment -

        Alan Woodward I think you've added a second TestEmbeddedSolrServer under the same package, different folder though. My eclipse doesn't compile (clean trunk checkout), and complains that these two exist:

        solr/solrj/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServer.java
        solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServer.java
        

        One under solr/solrj and one under solr/core.

        Show
        Shai Erera added a comment - Alan Woodward I think you've added a second TestEmbeddedSolrServer under the same package, different folder though. My eclipse doesn't compile (clean trunk checkout), and complains that these two exist: solr/solrj/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServer.java solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServer.java One under solr/solrj and one under solr/core.
        Hide
        Alan Woodward added a comment -

        Huh, how odd. Ant and IntelliJ don't seem to have a problem with it. I'll rename the new one though.

        Show
        Alan Woodward added a comment - Huh, how odd. Ant and IntelliJ don't seem to have a problem with it. I'll rename the new one though.
        Hide
        ASF subversion and git services added a comment -

        Commit 1669427 from Alan Woodward in branch 'dev/trunk'
        [ https://svn.apache.org/r1669427 ]

        SOLR-7307: Rename test file to make Eclipse happy

        Show
        ASF subversion and git services added a comment - Commit 1669427 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1669427 ] SOLR-7307 : Rename test file to make Eclipse happy
        Hide
        ASF subversion and git services added a comment -

        Commit 1669429 from Alan Woodward in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1669429 ]

        SOLR-7307: Rename test file to make Eclipse happy

        Show
        ASF subversion and git services added a comment - Commit 1669429 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1669429 ] SOLR-7307 : Rename test file to make Eclipse happy
        Hide
        Shai Erera added a comment -

        Or, perhaps they could be merged into one? Anyway, your call.

        Show
        Shai Erera added a comment - Or, perhaps they could be merged into one? Anyway, your call.

          People

          • Assignee:
            Alan Woodward
            Reporter:
            Alan Woodward
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development