Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.6
    • Component/s: Schema and Analysis
    • Labels:
      None

      Description

      Sub task of SOLR-7041.

      For new configs with luceneMatchVersion >=6.6.0 we'll throw an exception if defaultSearchField is used in schema.

      For luceneMatchVersion < 6.0.0 we'll behave like before (warn only). This is to deliver on our back-compat promise between minor versions.

      1. SOLR-10587.patch
        5 kB
        Jan Høydahl
      2. SOLR-10587.patch
        5 kB
        Jason Gerlowski
      3. SOLR-10587.patch
        46 kB
        Jason Gerlowski

        Activity

        Hide
        gerlowskija Jason Gerlowski added a comment -

        Attached patch makes the required changes to IndexSchema. A few notes:

        • includes a negative test in BadIndexSchemaTest for the combination of defaultSearchField and luceneMatchVersion
        • there are some tests that fail with this patch on current master or branch_6x. There's a good number of schema's which still include defaultSearchField. As I understand what I read on SOLR-7041, these test failures are "expected" for now, as the work to change these schemas is still ongoing. Just wanted to mention them here explicitly as a disclaimer, or in case I'm misunderstanding something and there's actual issues with the patch.

        As a side note, in writing this patch, I noticed that most/all of the error cases in IndexSchema throw a SolrException with 500/ServerError. I'm not entirely sure how these status codes propagate out of this layer; maybe the status codes get changed in some cases before being returned to users. But it might be more intuitive/correct to expose these as 400's/BadRequest. I know that runs into deprecation concerns, and all that. Just wanted to check my understanding and make sure I was reading things correctly before creating a JIRA.

        Show
        gerlowskija Jason Gerlowski added a comment - Attached patch makes the required changes to IndexSchema . A few notes: includes a negative test in BadIndexSchemaTest for the combination of defaultSearchField and luceneMatchVersion there are some tests that fail with this patch on current master or branch_6x. There's a good number of schema's which still include defaultSearchField. As I understand what I read on SOLR-7041 , these test failures are "expected" for now, as the work to change these schemas is still ongoing. Just wanted to mention them here explicitly as a disclaimer, or in case I'm misunderstanding something and there's actual issues with the patch. As a side note, in writing this patch, I noticed that most/all of the error cases in IndexSchema throw a SolrException with 500/ServerError. I'm not entirely sure how these status codes propagate out of this layer; maybe the status codes get changed in some cases before being returned to users. But it might be more intuitive/correct to expose these as 400's/BadRequest. I know that runs into deprecation concerns, and all that. Just wanted to check my understanding and make sure I was reading things correctly before creating a JIRA.
        Hide
        janhoy Jan Høydahl added a comment -

        Thanks for contributing. Comments:

        • We should keep the WARNING about deprecation, the one you changed from WARN to INFO
        • bad-schema-defaultsearchfield.xml can be much smaller, e.g.
          <schema name="bad-schema-defaultsearchfield" version="1.6">
            <fieldType name="string" class="solr.StrField"/>
            <field name="id" type="string" indexed="true" stored="true" multiValued="false" required="false"/>
            <uniqueKey>id</uniqueKey>
            <!-- BEGIN BAD STUFF: not allowed anymore -->
            <defaultSearchField>id</defaultSearchField>
            <!-- END BAD STUFF -->
          </schema>
          

        Would you like to submit an updated patch?

        Wrt converting existing tests into using "df" I think we can use SOLR-7041 for that and commit same to both master and branch_6x. I think we should just do a series of smaller commits until all tests are converted, and once that is done this one can be committed.

        Show
        janhoy Jan Høydahl added a comment - Thanks for contributing. Comments: We should keep the WARNING about deprecation, the one you changed from WARN to INFO bad-schema-defaultsearchfield.xml can be much smaller, e.g. <schema name= "bad-schema-defaultsearchfield" version= "1.6" > <fieldType name= "string" class= "solr.StrField" /> <field name= "id" type= "string" indexed= "true" stored= "true" multiValued= "false" required= "false" /> <uniqueKey> id </uniqueKey> <!-- BEGIN BAD STUFF: not allowed anymore --> <defaultSearchField> id </defaultSearchField> <!-- END BAD STUFF --> </schema> Would you like to submit an updated patch? Wrt converting existing tests into using "df" I think we can use SOLR-7041 for that and commit same to both master and branch_6x. I think we should just do a series of smaller commits until all tests are converted, and once that is done this one can be committed.
        Hide
        gerlowskija Jason Gerlowski added a comment -

        Updated patch addresses your comments. I trimmed down the schema.xml file as you proposed. The WARNING message you asked about is still there in this patch, but has been moved to the checkVersionCompatibilityForDefaultSearchField method, as it seemed to fit there.

        Show
        gerlowskija Jason Gerlowski added a comment - Updated patch addresses your comments. I trimmed down the schema.xml file as you proposed. The WARNING message you asked about is still there in this patch, but has been moved to the checkVersionCompatibilityForDefaultSearchField method, as it seemed to fit there.
        Hide
        gerlowskija Jason Gerlowski added a comment -

        If you're not happy with the move of the log message, I am more than happy to change it back to living in readSchema or wherever else you'd prefer.

        Show
        gerlowskija Jason Gerlowski added a comment - If you're not happy with the move of the log message, I am more than happy to change it back to living in readSchema or wherever else you'd prefer.
        Hide
        janhoy Jan Høydahl added a comment -

        Thanks. Think this is more or less what we need. I'll try to switch tests to using "df" and then commit this.

        Show
        janhoy Jan Høydahl added a comment - Thanks. Think this is more or less what we need. I'll try to switch tests to using "df" and then commit this.
        Hide
        janhoy Jan Høydahl added a comment -

        Updated patch with added test for back-compat <6.6, added CHANGES entry.
        All tests pass, after last commit cutting over tests to using 'df'.
        Precommit passes.

        Show
        janhoy Jan Høydahl added a comment - Updated patch with added test for back-compat <6.6, added CHANGES entry. All tests pass, after last commit cutting over tests to using 'df'. Precommit passes.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit d2ea33d7687f38894e03217e076e002dbfe66b76 in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d2ea33d ]

        SOLR-10587: Ban defaultSearchField in schema for luceneMatchVersion =>6.6.0

        Show
        jira-bot ASF subversion and git services added a comment - Commit d2ea33d7687f38894e03217e076e002dbfe66b76 in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d2ea33d ] SOLR-10587 : Ban defaultSearchField in schema for luceneMatchVersion =>6.6.0
        Hide
        janhoy Jan Høydahl added a comment -

        Committed to 6x. Thanks Jason Gerlowski

        Show
        janhoy Jan Høydahl added a comment - Committed to 6x. Thanks Jason Gerlowski
        Hide
        mkhludnev Mikhail Khludnev added a comment -

        Hello, Jan Høydahl
        These days BadIndexSchemaTest noises a lot. I've checked one output
        https://jenkins.thetaphi.de/job/Lucene-Solr-6.x-Linux/3454/consoleFull
        The smoking gun is

           [junit4]   2> 211623 INFO  (searcherExecutor-962-thread-1) [    ] o.a.s.c.SolrCore [collection1] Registered new searcher Searcher@1e5f10d[collection1] main{ExitableDirectoryReader(UninvertingDirectoryReader())}
           [junit4]   2> 211624 INFO  (TEST-BadIndexSchemaTest.testDefaultSearchFieldNotBannedForPre66-seed#[880DCFD8CCD8713A]) [    ] o.a.s.SolrTestCaseJ4 ####initCore end
        ...
        
           [junit4]   2> May 06, 2017 8:20:27 PM com.carrotsearch.randomizedtesting.ThreadLeakControl tryToInterruptAll
           [junit4]   2> SEVERE: There are still zombie threads that couldn't be terminated:
           [junit4]   2>    1) Thread[id=1938, name=searcherExecutor-962-thread-1, state=WAITING, group=TGRP-BadIndexSchemaTest]
           [junit4]   2>         at java.base@9-ea/jdk.internal.misc.Unsafe.park(Native Method)
           [junit4]   2>         at java.base@9-ea/java.util.concurrent.locks.LockSupport.park(LockSupport.java:192)
           [junit4]   2>         at java.base@9-ea/java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2062)
           [junit4]   2>         at java.base@9-ea/java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:435)
           [junit4]   2>         at java.base@9-ea/java.util.concurrent.ThreadPoolExecutor.getTask(ThreadPoolExecutor.java:1086)
           [junit4]   2>         at java.base@9-ea/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146)
           [junit4]   2>         at java.base@9-ea/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
           [junit4]   2>         at java.base@9-ea/java.lang.Thread.run(Thread.java:844)
        ..
        ant test  -Dtestcase=BadIndexSchemaTest -Dtests.seed=880DCFD8CCD8713A -Dtests.multiplier=3 -Dtests.slow=true -Dtests.locale=es-HN -Dtests.timezone=Australia/Melbourne -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
        

        this seed reproduces to me at branch_6x and jdk 8

        Show
        mkhludnev Mikhail Khludnev added a comment - Hello, Jan Høydahl These days BadIndexSchemaTest noises a lot. I've checked one output https://jenkins.thetaphi.de/job/Lucene-Solr-6.x-Linux/3454/consoleFull The smoking gun is [junit4] 2> 211623 INFO (searcherExecutor-962-thread-1) [ ] o.a.s.c.SolrCore [collection1] Registered new searcher Searcher@1e5f10d[collection1] main{ExitableDirectoryReader(UninvertingDirectoryReader())} [junit4] 2> 211624 INFO (TEST-BadIndexSchemaTest.testDefaultSearchFieldNotBannedForPre66-seed#[880DCFD8CCD8713A]) [ ] o.a.s.SolrTestCaseJ4 ####initCore end ... [junit4] 2> May 06, 2017 8:20:27 PM com.carrotsearch.randomizedtesting.ThreadLeakControl tryToInterruptAll [junit4] 2> SEVERE: There are still zombie threads that couldn't be terminated: [junit4] 2> 1) Thread [id=1938, name=searcherExecutor-962-thread-1, state=WAITING, group=TGRP-BadIndexSchemaTest] [junit4] 2> at java.base@9-ea/jdk.internal.misc.Unsafe.park(Native Method) [junit4] 2> at java.base@9-ea/java.util.concurrent.locks.LockSupport.park(LockSupport.java:192) [junit4] 2> at java.base@9-ea/java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2062) [junit4] 2> at java.base@9-ea/java.util.concurrent.LinkedBlockingQueue.take(LinkedBlockingQueue.java:435) [junit4] 2> at java.base@9-ea/java.util.concurrent.ThreadPoolExecutor.getTask(ThreadPoolExecutor.java:1086) [junit4] 2> at java.base@9-ea/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146) [junit4] 2> at java.base@9-ea/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [junit4] 2> at java.base@9-ea/java.lang. Thread .run( Thread .java:844) .. ant test -Dtestcase=BadIndexSchemaTest -Dtests.seed=880DCFD8CCD8713A -Dtests.multiplier=3 -Dtests.slow= true -Dtests.locale=es-HN -Dtests.timezone=Australia/Melbourne -Dtests.asserts= true -Dtests.file.encoding=US-ASCII this seed reproduces to me at branch_6x and jdk 8
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 56a5ba03a21b0846977ac32594461a23c34f3b72 in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=56a5ba0 ]

        SOLR-10587: Fix test errors by properly calling deleteCore() for the back-compat tests

        Show
        jira-bot ASF subversion and git services added a comment - Commit 56a5ba03a21b0846977ac32594461a23c34f3b72 in lucene-solr's branch refs/heads/branch_6x from Jan Høydahl [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=56a5ba0 ] SOLR-10587 : Fix test errors by properly calling deleteCore() for the back-compat tests
        Hide
        janhoy Jan Høydahl added a comment -

        Thanks Mikhail Khludnev, think I've nailed it

        Show
        janhoy Jan Høydahl added a comment - Thanks Mikhail Khludnev , think I've nailed it

          People

          • Assignee:
            Unassigned
            Reporter:
            janhoy Jan Høydahl
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development