Details

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

      Description

      we are working on improving some functionality in lucene's build/tests, it would be good to improve the solr side to take advantage of it.
      currently its only sorta-kinda integrated and a bit messy.
      i'd like to do some incremental improvements piece-by-piece on this issue.

      1. SOLR-2002_buildfailed.patch
        2 kB
        Robert Muir
      2. SOLR-2002_core_contrib.patch
        2 kB
        Robert Muir
      3. SOLR-2002_dih.patch
        149 kB
        Robert Muir
      4. SOLR-2002_lbhttpsolrserver.patch
        5 kB
        Robert Muir
      5. SOLR-2002_localization.patch
        9 kB
        Robert Muir
      6. SOLR-2002_lucenetestcase.patch
        51 kB
        Robert Muir
      7. SOLR-2002_merged.patch
        103 kB
        Robert Muir
      8. SOLR-2002_merged.patch
        106 kB
        Robert Muir
      9. SOLR-2002_merged.patch
        113 kB
        Steve Rowe
      10. SOLR-2002_merged.patch
        106 kB
        Robert Muir
      11. SOLR-2002_replication.patch
        16 kB
        Robert Muir
      12. SOLR-2002_testiter.patch
        7 kB
        Robert Muir
      13. SOLR-2002_testmethod.patch
        7 kB
        Robert Muir
      14. SOLR-2002_timeout.patch
        1 kB
        Robert Muir
      15. SOLR-2002.junit.xml.formatter.patch
        0.8 kB
        Hoss Man
      16. SOLR-2002.patch
        73 kB
        Robert Muir
      17. SOLR-2002setupteardown.patch
        4 kB
        Robert Muir
      18. TEST-org.apache.solr.spelling.suggest.SuggesterTest.txt
        19 kB
        Yonik Seeley

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          patch that cleans up tests a bit:

          • it makes AbstractSolrTestCase extends LuceneTestCase
          • fixes a few bugs in some tests (e.g. not calling super.setUp)

          i'd like to commit this shortly

          Show
          Robert Muir added a comment - patch that cleans up tests a bit: it makes AbstractSolrTestCase extends LuceneTestCase fixes a few bugs in some tests (e.g. not calling super.setUp) i'd like to commit this shortly
          Hide
          Robert Muir added a comment -

          ok, I committed the first patch (963873 trunk, 963877 3x).

          One good thing TODO: if solr has any random tests, it can now use the newRandom etc in case it fails, so its easy to reproduce.

          Show
          Robert Muir added a comment - ok, I committed the first patch (963873 trunk, 963877 3x). One good thing TODO: if solr has any random tests, it can now use the newRandom etc in case it fails, so its easy to reproduce.
          Hide
          Robert Muir added a comment -

          when switching solr's base tests to extend lucene's, i noticed if you override setUp(), but neglected to call super.setUp(), you would get a strange exception:
          java.lang.IllegalArgumentException: maxClauseCount must be >= 1
          http://www.lucidimagination.com/search/document/f7769140c3557c78/error_in_building_solr_cloud_ant_example

          This is because the code in tearDown to restore the max clause count restores an uninitialized (zero) saved clause count.
          (I actually found this helpful once i understood what was happening).

          This patch changes this to explicit asserts, which are more helpful and tell you if you forgot to call super.setUp() or super.tearDown() when you override these methods.

          Show
          Robert Muir added a comment - when switching solr's base tests to extend lucene's, i noticed if you override setUp(), but neglected to call super.setUp(), you would get a strange exception: java.lang.IllegalArgumentException: maxClauseCount must be >= 1 http://www.lucidimagination.com/search/document/f7769140c3557c78/error_in_building_solr_cloud_ant_example This is because the code in tearDown to restore the max clause count restores an uninitialized (zero) saved clause count. (I actually found this helpful once i understood what was happening). This patch changes this to explicit asserts, which are more helpful and tell you if you forgot to call super.setUp() or super.tearDown() when you override these methods.
          Hide
          Robert Muir added a comment -

          Committed SOLR-2002setupteardown.patch revisions 964459 (trunk) 964461 (3x)

          Show
          Robert Muir added a comment - Committed SOLR-2002 setupteardown.patch revisions 964459 (trunk) 964461 (3x)
          Hide
          Robert Muir added a comment -

          there is one single test in solr core that depends on contrib modules.
          I am confused about this test, as it seems to already test the functionality with core code (CSV) only to then also test it with contrib code (PDF)
          In my opinion: core tests shouldnt depend on contrib modules.

          I propose either:

          1. applying this patch, and removing the dependency of core tests on contrib.
          2. if this functionality (extraction) is considered core, then it should be moved out of contrib.

          Can someone take a look and give me their opinion? I think this would really start to make it easier to improve the build.

          Show
          Robert Muir added a comment - there is one single test in solr core that depends on contrib modules. I am confused about this test, as it seems to already test the functionality with core code (CSV) only to then also test it with contrib code (PDF) In my opinion: core tests shouldnt depend on contrib modules. I propose either: applying this patch, and removing the dependency of core tests on contrib. if this functionality (extraction) is considered core, then it should be moved out of contrib. Can someone take a look and give me their opinion? I think this would really start to make it easier to improve the build.
          Hide
          Robert Muir added a comment -

          Committed revision 964753 to trunk. However, I can't yet merge it to 3x, because in 3x the jetty tests use the example config, which wants to load of contrib/velocity (its in core in trunk, so no problem).

          I think the fix is for these tests, to not use the example config but copy it over to test-files and disable VRW.

            // Try not introduce a dependency on the example schema or config unless you need to.
            // using configs in the test directory allows more flexibility to change "example"
            // without breaking configs.
          
            public static String EXAMPLE_HOME="../../../example/solr/";
            public static String EXAMPLE_SCHEMA=EXAMPLE_HOME+"conf/schema.xml";
            public static String EXAMPLE_CONFIG=EXAMPLE_HOME+"conf/solrconfig.xml";
          
          Show
          Robert Muir added a comment - Committed revision 964753 to trunk. However, I can't yet merge it to 3x, because in 3x the jetty tests use the example config, which wants to load of contrib/velocity (its in core in trunk, so no problem). I think the fix is for these tests, to not use the example config but copy it over to test-files and disable VRW. // Try not introduce a dependency on the example schema or config unless you need to. // using configs in the test directory allows more flexibility to change "example" // without breaking configs. public static String EXAMPLE_HOME= "../../../example/solr/" ; public static String EXAMPLE_SCHEMA=EXAMPLE_HOME+ "conf/schema.xml" ; public static String EXAMPLE_CONFIG=EXAMPLE_HOME+ "conf/solrconfig.xml" ;
          Hide
          Robert Muir added a comment -

          ok, after backporting velocity changes, committed revision 964826 to 3x.

          we could still consider in the future if these jetty tests should use the example schema or their own in test-files

          Show
          Robert Muir added a comment - ok, after backporting velocity changes, committed revision 964826 to 3x. we could still consider in the future if these jetty tests should use the example schema or their own in test-files
          Hide
          Hoss Man added a comment -

          we could still consider in the future if these jetty tests should use the example schema or their own in test-files

          the purpose of those tests is not just to test Jetty – it's to test that the example configs load and are usable.

          Show
          Hoss Man added a comment - we could still consider in the future if these jetty tests should use the example schema or their own in test-files the purpose of those tests is not just to test Jetty – it's to test that the example configs load and are usable.
          Hide
          Robert Muir added a comment -

          the purpose of those tests is not just to test Jetty - it's to test that the example configs load and are usable.

          This is messy and should be fixed: these are jetty tests. If you truly believe this (I think you are wrong), then we should do the following:

          If the example config should be tested to be loaded and usable:

          1. it should not be mixed in jetty tests, this is wrong! (as the comment describes)
          2. we should create a TestExampleConfig, and its purpose is to test various things about the example config, nothing more.
          3. the example config used in said tests shouldnt load up shit from contrib.
          Show
          Robert Muir added a comment - the purpose of those tests is not just to test Jetty - it's to test that the example configs load and are usable. This is messy and should be fixed: these are jetty tests. If you truly believe this (I think you are wrong), then we should do the following: If the example config should be tested to be loaded and usable: it should not be mixed in jetty tests, this is wrong! (as the comment describes) we should create a TestExampleConfig, and its purpose is to test various things about the example config, nothing more. the example config used in said tests shouldnt load up shit from contrib.
          Hide
          Robert Muir added a comment -

          To further my point, I will say that these "example tests" failed last night on both branches so if they are really unit tests as you claim, then the example config sucks, because it fails often.

          This is why i think the two concerns should be separate: or it isnt a damn unit test at all.

          Show
          Robert Muir added a comment - To further my point, I will say that these "example tests" failed last night on both branches so if they are really unit tests as you claim, then the example config sucks, because it fails often. This is why i think the two concerns should be separate: or it isnt a damn unit test at all.
          Hide
          Hoss Man added a comment -

          If you truly believe this (I think you are wrong), then we should do the following:

          it's right there in the javadoc for SolrExampleTestBase

           * This should include tests against the example solr config
          

          ...it has subclasses that use Jetty, and other subclasses that use embedded solr.

          the example config used in said tests shouldnt load up shit from contrib.

          the example config is the example config – it's the config we put up on a pedestal in each release and say "this is what you should start with when writing your own config". we should test that those configs are actually usable w/o errors – that was the goal of the test

          If the example configs suggest loading stuff from contrib, then we need to test that too.

          ...if they are really unit tests as you claim...

          a) I never claimed they were unit tests ... i called them "tests".
          b) I have no interest in getting into a philosophical debate about unit vs functional vs integration tests.
          c) I never stated an opinion about whether we need to test the usage of these configs in both in embedded clients and in jetty.

          i didn't write the test, and i didn't defend the use of Jetty for this – so pick a fight with someone else if that's your goal. All i did was point out that changing the test to use configs different from those in the example directory defeats the point of the tests.

          Show
          Hoss Man added a comment - If you truly believe this (I think you are wrong), then we should do the following: it's right there in the javadoc for SolrExampleTestBase * This should include tests against the example solr config ...it has subclasses that use Jetty, and other subclasses that use embedded solr. the example config used in said tests shouldnt load up shit from contrib. the example config is the example config – it's the config we put up on a pedestal in each release and say "this is what you should start with when writing your own config". we should test that those configs are actually usable w/o errors – that was the goal of the test If the example configs suggest loading stuff from contrib, then we need to test that too. ...if they are really unit tests as you claim... a) I never claimed they were unit tests ... i called them "tests". b) I have no interest in getting into a philosophical debate about unit vs functional vs integration tests. c) I never stated an opinion about whether we need to test the usage of these configs in both in embedded clients and in jetty. i didn't write the test, and i didn't defend the use of Jetty for this – so pick a fight with someone else if that's your goal. All i did was point out that changing the test to use configs different from those in the example directory defeats the point of the tests.
          Hide
          Robert Muir added a comment -

          it's right there in the javadoc for SolrExampleTestBase

          I'm not talking about Solr*Example*TestBase

          I am talking about Solr*Jetty*TestBase:
          its right here in the javadoc for SolrJettyTestBase:

            // Try not introduce a dependency on the example schema or config unless you need to.
            // using configs in the test directory allows more flexibility to change "example"
            // without breaking configs.
          

          I pasted this before, but you are perhaps confused with Solr*Example*TestBase

          Show
          Robert Muir added a comment - it's right there in the javadoc for SolrExampleTestBase I'm not talking about Solr*Example*TestBase I am talking about Solr*Jetty*TestBase: its right here in the javadoc for SolrJettyTestBase: // Try not introduce a dependency on the example schema or config unless you need to. // using configs in the test directory allows more flexibility to change "example" // without breaking configs. I pasted this before, but you are perhaps confused with Solr*Example*TestBase
          Hide
          Hoss Man added a comment -

          Actually i got confused and thought you were talking about SolrExampleJettyTest which is a decendent of SolrExampleTestBase – and i thought the comment you quoted was something you added (or were proposing be added).

          Definitely my mistake.

          If some "Example" tests exist, and verify that the example configs work, then by all means we should change any other tests that currently use "example/solr/conf" to use more isolated config files.

          I've got no problem with that.

          Show
          Hoss Man added a comment - Actually i got confused and thought you were talking about SolrExampleJettyTest which is a decendent of SolrExampleTestBase – and i thought the comment you quoted was something you added (or were proposing be added). Definitely my mistake. If some "Example" tests exist, and verify that the example configs work, then by all means we should change any other tests that currently use "example/solr/conf" to use more isolated config files. I've got no problem with that.
          Hide
          Yonik Seeley added a comment -

          Yeah, I added SolrJettyTestBase (and those comments) back in March cause I was sick of trying to change the example schema just to have loads of tests fail that should not have been depending on example. I don't think I got as far as actually moving many existing tests off of example though.

          Show
          Yonik Seeley added a comment - Yeah, I added SolrJettyTestBase (and those comments) back in March cause I was sick of trying to change the example schema just to have loads of tests fail that should not have been depending on example. I don't think I got as far as actually moving many existing tests off of example though.
          Hide
          Robert Muir added a comment -

          today this test (SolrRequestParserTest) was hanging infinitely because it wanted to connect to http://www.apache.org/dist/lucene/solr/. it was connecting but waiting for data infinitely.

          this patch adds some reasonable (5s) timeouts to connect/read so it will fail instead of hanging
          for > 15 minutes (before i killed it)

          Show
          Robert Muir added a comment - today this test (SolrRequestParserTest) was hanging infinitely because it wanted to connect to http://www.apache.org/dist/lucene/solr/ . it was connecting but waiting for data infinitely. this patch adds some reasonable (5s) timeouts to connect/read so it will fail instead of hanging for > 15 minutes (before i killed it)
          Hide
          Robert Muir added a comment -

          attached is a patch with improvements to TestReplicationHandler:

          • speeding up the test and making it more reliable (see rQuery)
          • removing all scary warnings
          • closing a previously unclosed reader that was preventing it from cleaning up after itself.

          would appreciate anyone thats willing to apply the patch and try running the test a few times to see if its more stable for them.

          Show
          Robert Muir added a comment - attached is a patch with improvements to TestReplicationHandler: speeding up the test and making it more reliable (see rQuery) removing all scary warnings closing a previously unclosed reader that was preventing it from cleaning up after itself. would appreciate anyone thats willing to apply the patch and try running the test a few times to see if its more stable for them.
          Hide
          Robert Muir added a comment -

          Committed the replication test improvements, revision 965327 965328 (3x)

          If there are problems, we should just go back, but for me this fixes many problems (such as newname not found etc etc)

          Show
          Robert Muir added a comment - Committed the replication test improvements, revision 965327 965328 (3x) If there are problems, we should just go back, but for me this fixes many problems (such as newname not found etc etc)
          Hide
          Robert Muir added a comment -

          attached is a patch to vary locale and timezone randomly in tests, just like codecs and other defaults.

          Show
          Robert Muir added a comment - attached is a patch to vary locale and timezone randomly in tests, just like codecs and other defaults.
          Hide
          Robert Muir added a comment -

          committed SOLR-2002_localization.patch revision 983495 (trunk) 983499 (3x)

          Show
          Robert Muir added a comment - committed SOLR-2002 _localization.patch revision 983495 (trunk) 983499 (3x)
          Hide
          Robert Muir added a comment -

          attached patch switches remaining 'extends TestCase' to 'extends LuceneTestCase' for better coverage.

          Show
          Robert Muir added a comment - attached patch switches remaining 'extends TestCase' to 'extends LuceneTestCase' for better coverage.
          Hide
          Robert Muir added a comment -

          committed SOLR-2002_lucenetestcase.patch revision 983530 (trunk) 983533 (3x)

          Show
          Robert Muir added a comment - committed SOLR-2002 _lucenetestcase.patch revision 983530 (trunk) 983533 (3x)
          Hide
          Robert Muir added a comment -

          (yeah i know i am abusing this issue)

          This patch adds -Dtestmethod, in case you just want to run a single test method.

          Example:

           ant test-core -Dtestcase=TestIndexWriter -Dtestmethod=testDocCount
          ...
          junit-sequential:
              [junit] Testsuite: org.apache.lucene.index.TestIndexWriter
              [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.672 sec
          
          Show
          Robert Muir added a comment - (yeah i know i am abusing this issue) This patch adds -Dtestmethod, in case you just want to run a single test method. Example: ant test-core -Dtestcase=TestIndexWriter -Dtestmethod=testDocCount ... junit-sequential: [junit] Testsuite: org.apache.lucene.index.TestIndexWriter [junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.672 sec
          Hide
          Michael McCandless added a comment -

          FANTASTIC!

          I've wanted this method for a VERY long time...

          Show
          Michael McCandless added a comment - FANTASTIC! I've wanted this method for a VERY long time...
          Hide
          Robert Muir added a comment -

          attached is a patch for a -Dtests.iter

          This lets you easily run a test multiple times without writing a loop in the shell or whatever, for example:

          ant test -Dtestcase=TestRegexpRandom2 -Dtests.iter=3
          ant test -Dtestcase=TestNumericRangeQuery32 -Dtestmethod=testInverseRange -Dtests.iter=100
          

          its useful for random tests, but i dont recommend running ant test -Dtests.iter=2 for example, because some tests aren't well behaved. but these can always be fixed at some later time, too.

          Show
          Robert Muir added a comment - attached is a patch for a -Dtests.iter This lets you easily run a test multiple times without writing a loop in the shell or whatever, for example: ant test -Dtestcase=TestRegexpRandom2 -Dtests.iter=3 ant test -Dtestcase=TestNumericRangeQuery32 -Dtestmethod=testInverseRange -Dtests.iter=100 its useful for random tests, but i dont recommend running ant test -Dtests.iter=2 for example, because some tests aren't well behaved. but these can always be fixed at some later time, too.
          Hide
          Robert Muir added a comment -

          committed -Dtests.iter in revision 989321.

          Show
          Robert Muir added a comment - committed -Dtests.iter in revision 989321.
          Hide
          Robert Muir added a comment -

          since we merged lucene & solr, the build system has been somewhat of a mess.

          attached is a very early patch thats basically a reboot of the solr build:

          • it reuses the logic from lucene's build
          • its significantly faster, especially dependencies with lucene's up2date macros
          • its nowhere near committable yet

          One interesting thing found so far the solr contribs basically have their own build systems: and they are hiding exceptions going on behind the scenes when running tests (try the patch to see)

          The patch doesnt yet work for things like 'dist' or 'example'. at the moment only things like 'ant compile, ant test, ant javadocs' work correctly.

          additionally the contrib/dataimporthandler 'extras' isnt compiled or tested yet, I think i would like to propose instead we make contrib/dataimporthandler-extras, that depends on the main dataimporthandler, this would really simplify the build.

          Show
          Robert Muir added a comment - since we merged lucene & solr, the build system has been somewhat of a mess. attached is a very early patch thats basically a reboot of the solr build: it reuses the logic from lucene's build its significantly faster, especially dependencies with lucene's up2date macros its nowhere near committable yet One interesting thing found so far the solr contribs basically have their own build systems: and they are hiding exceptions going on behind the scenes when running tests (try the patch to see) The patch doesnt yet work for things like 'dist' or 'example'. at the moment only things like 'ant compile, ant test, ant javadocs' work correctly. additionally the contrib/dataimporthandler 'extras' isnt compiled or tested yet, I think i would like to propose instead we make contrib/dataimporthandler-extras, that depends on the main dataimporthandler, this would really simplify the build.
          Hide
          Robert Muir added a comment -

          by the way, i think this really simplifies the contrib builds.

          its probably hard to see in the patch: but here is the entire contrib/clustering build now

          <project name="solr-clustering" default="default">
            <description>Clustering Integration.</description>
          
            <property name="src.dir" location="src/main/java"/>
            <property name="tests.src.dir" location="src/test/java"/>
            <property name="tests.userdir" location="src/test/resources"/>
          
            <import file="../contrib-build.xml"/>
          </project>
          
          Show
          Robert Muir added a comment - by the way, i think this really simplifies the contrib builds. its probably hard to see in the patch: but here is the entire contrib/clustering build now <project name="solr-clustering" default="default"> <description>Clustering Integration.</description> <property name="src.dir" location="src/main/java"/> <property name="tests.src.dir" location="src/test/java"/> <property name="tests.userdir" location="src/test/resources"/> <import file="../contrib-build.xml"/> </project>
          Hide
          Yonik Seeley added a comment -

          Sounds cool! Whatever those strong in ant-foo come up with is fine with me!

          Show
          Yonik Seeley added a comment - Sounds cool! Whatever those strong in ant-foo come up with is fine with me!
          Hide
          Robert Muir added a comment -

          thanks, the major thing left is to consolidate release management-type things (e.g. rat reporting tasks, dist/packaging, artifact signing, checksumming, etc).

          most of this is really inappropriate the way it is in lucene's build, because its standalone in lucene's build.xml and not reusable to modules and solr. for example: 'rat-sources' just runs on a hardcoded "src/java" for lucene-core.

          so we need to fix this kind of stuff anyway so that things in modules/ can actually ever release, no way to do this at the moment.

          Show
          Robert Muir added a comment - thanks, the major thing left is to consolidate release management-type things (e.g. rat reporting tasks, dist/packaging, artifact signing, checksumming, etc). most of this is really inappropriate the way it is in lucene's build, because its standalone in lucene's build.xml and not reusable to modules and solr. for example: 'rat-sources' just runs on a hardcoded "src/java" for lucene-core. so we need to fix this kind of stuff anyway so that things in modules/ can actually ever release, no way to do this at the moment.
          Hide
          Steve Rowe added a comment - - edited

          Attached patch syncs up to current trunk, and includes dataimporthandler-extras in the build, by moving it to its own directory under solr/contrib/. All tests run and pass.

          Run the following script before applying the patch, which was created using svn --no-diff-deleted diff:

          mkdir -p solr/contrib/dataimporthandler-extras/src
          svn move solr/contrib/dataimporthandler/src/extras/main/ solr/contrib/dataimporthandler-extras/src/main
          svn move solr/contrib/dataimporthandler/src/extras/test solr/contrib/dataimporthandler-extras/src/test
          

          solr/contrib/dataimporthandler-extras/build.xml is a lot more complicated than the other Solr contrib build files, because it has non-standard dependencies at compile and test time. I think this can be cleaned up and consolidated, e.g. with some of the stuff in solr/build.xml - the attached patch is just what it took to get it to work.

          Show
          Steve Rowe added a comment - - edited Attached patch syncs up to current trunk, and includes dataimporthandler-extras in the build, by moving it to its own directory under solr/contrib/ . All tests run and pass. Run the following script before applying the patch, which was created using svn --no-diff-deleted diff : mkdir -p solr/contrib/dataimporthandler-extras/src svn move solr/contrib/dataimporthandler/src/extras/main/ solr/contrib/dataimporthandler-extras/src/main svn move solr/contrib/dataimporthandler/src/extras/test solr/contrib/dataimporthandler-extras/src/test solr/contrib/dataimporthandler-extras/build.xml is a lot more complicated than the other Solr contrib build files, because it has non-standard dependencies at compile and test time. I think this can be cleaned up and consolidated, e.g. with some of the stuff in solr/build.xml - the attached patch is just what it took to get it to work.
          Hide
          Robert Muir added a comment -

          Thank you for merging this patch up!
          I was dreading it, but this is an important issue and I think is holding up other good improvements.

          Show
          Robert Muir added a comment - Thank you for merging this patch up! I was dreading it, but this is an important issue and I think is holding up other good improvements.
          Hide
          Robert Muir added a comment -

          i committed the lucene bits here to make at least small progress.

          attached is the updated patch.

          Show
          Robert Muir added a comment - i committed the lucene bits here to make at least small progress. attached is the updated patch.
          Hide
          Steve Rowe added a comment -

          Hmm, I didn't get the pre-patch script fully correct - here's what it should look like:

          mkdir -p solr/contrib/dataimporthandler-extras/src
          svn add solr/contrib/dataimporthandler-extras/
          svn move solr/contrib/dataimporthandler/src/extras/main/ solr/contrib/dataimporthandler-extras/src/main
          svn move solr/contrib/dataimporthandler/src/extras/test solr/contrib/dataimporthandler-extras/src/test
          svn rm solr/contrib/dataimporthandler/src/extras
          
          Show
          Steve Rowe added a comment - Hmm, I didn't get the pre-patch script fully correct - here's what it should look like: mkdir -p solr/contrib/dataimporthandler-extras/src svn add solr/contrib/dataimporthandler-extras/ svn move solr/contrib/dataimporthandler/src/extras/main/ solr/contrib/dataimporthandler-extras/src/main svn move solr/contrib/dataimporthandler/src/extras/test solr/contrib/dataimporthandler-extras/src/test svn rm solr/contrib/dataimporthandler/src/extras
          Hide
          Robert Muir added a comment -

          i simplified the dataimporthandler-extras contrib here.

          Show
          Robert Muir added a comment - i simplified the dataimporthandler-extras contrib here.
          Hide
          Steve Rowe added a comment -

          i simplified the dataimporthandler-extras contrib here.

          Very nice! I was able to successfully compile and run all tests from the top level, as well as the single-module auto-make-dependencies build from dataimporthandler-extras:

          ant clean # at top-level dir.
          cd solr/contrib/dataimporthandler-extras
          ant test
          
          Show
          Steve Rowe added a comment - i simplified the dataimporthandler-extras contrib here. Very nice! I was able to successfully compile and run all tests from the top level, as well as the single-module auto-make-dependencies build from dataimporthandler-extras: ant clean # at top-level dir. cd solr/contrib/dataimporthandler-extras ant test
          Hide
          Steve Rowe added a comment -

          I would like to make solr-core, solrj, and solr-webapp full-fledged separate modules (IntelliJ has difficulty importing the project from the Maven POMs at LUCENE-2657 - see https://issues.apache.org/jira/browse/LUCENE-2657?focusedCommentId=12919172&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12919172).

          I'm thinking of this structure:

          • solr/
            • core/
              • src/ from current src/java/
              • ...
            • solrj/
              • src/ from current src/common/ and src/solrj/
              • ...
            • webapp/
              • src/ from current src/webapp/src/
              • ...
          Show
          Steve Rowe added a comment - I would like to make solr-core, solrj, and solr-webapp full-fledged separate modules (IntelliJ has difficulty importing the project from the Maven POMs at LUCENE-2657 - see https://issues.apache.org/jira/browse/LUCENE-2657?focusedCommentId=12919172&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12919172 ). I'm thinking of this structure: solr/ core/ src/ from current src/java/ ... solrj/ src/ from current src/common/ and src/solrj/ ... webapp/ src/ from current src/webapp/src/ ...
          Hide
          Chris Male added a comment -

          +1 to your suggested layout Steven

          Show
          Chris Male added a comment - +1 to your suggested layout Steven
          Hide
          Robert Muir added a comment -

          +1 to fixing this situation, from the ant perspective.

          When a single build.xml is responsible for building multiple "targets", it creates a mess.

          Instead, if every folder is its own standalone module, then we could have a much simpler
          recursive build. Adding something new like findbugs integration would then be trivial, it could
          be added in one place and affect all of lucene/solr.

          Some suggestions:

          • I think common should be its own folder.
          • I think we should move lucene/src/demo to lucene/contrib/demo in 3.x like we did in trunk.
          Show
          Robert Muir added a comment - +1 to fixing this situation, from the ant perspective. When a single build.xml is responsible for building multiple "targets", it creates a mess. Instead, if every folder is its own standalone module, then we could have a much simpler recursive build. Adding something new like findbugs integration would then be trivial, it could be added in one place and affect all of lucene/solr. Some suggestions: I think common should be its own folder. I think we should move lucene/src/demo to lucene/contrib/demo in 3.x like we did in trunk.
          Hide
          Robert Muir added a comment -

          The TestLBHttpSolrServer is quite unreliable, especially with hudson builds.
          this is because it shutsdown a jetty on port X then restarts it on the same port again...

          attached is a patch that sets SO_REUSEADDR when running tests.

          Show
          Robert Muir added a comment - The TestLBHttpSolrServer is quite unreliable, especially with hudson builds. this is because it shutsdown a jetty on port X then restarts it on the same port again... attached is a patch that sets SO_REUSEADDR when running tests.
          Hide
          Robert Muir added a comment -

          Committed to trunk (revision 1021969). Also set the connect-timeout to 1s in 1021971.

          Merged both to 3x in revision 1021972.

          Sorry for the heavy committing, but this test was unreliable and took 15 minutes on hudson!

          Show
          Robert Muir added a comment - Committed to trunk (revision 1021969). Also set the connect-timeout to 1s in 1021971. Merged both to 3x in revision 1021972. Sorry for the heavy committing, but this test was unreliable and took 15 minutes on hudson!
          Hide
          Robert Muir added a comment -

          here's some cleanup to the DIH tests, isn't going to solve any of the test reliability
          problems we have, but I think it makes the tests simpler to work with as a start.

          Show
          Robert Muir added a comment - here's some cleanup to the DIH tests, isn't going to solve any of the test reliability problems we have, but I think it makes the tests simpler to work with as a start.
          Hide
          Yonik Seeley added a comment -

          SuggesterTest fails quite a bit for me...

              [junit] Testsuite: org.apache.solr.spelling.suggest.SuggesterTest
              [junit] Testcase: testSuggestions(org.apache.solr.spelling.suggest.SuggesterTest):  FAILED
              [junit] query failed XPath: //lst[@name='spellcheck']/lst[@name='suggestions']/lst[@name='ac']/int[@name='numFound'][.='2']
              [junit]  xml response was: <?xml version="1.0" encoding="UTF-8"?>
              [junit] <response>
              [junit] <lst name="responseHeader"><int name="status">0</int><int name="QTime">1</int></lst><lst name="spellcheck"><lst name="suggestions"/></lst>
              [junit] </response>
          

          Seems like the dict isn't being built for some reason, but it always seems to pass if I run the test individually.
          I'll try and reproduce with logging at INFO.

          Show
          Yonik Seeley added a comment - SuggesterTest fails quite a bit for me... [junit] Testsuite: org.apache.solr.spelling.suggest.SuggesterTest [junit] Testcase: testSuggestions(org.apache.solr.spelling.suggest.SuggesterTest): FAILED [junit] query failed XPath: //lst[@name='spellcheck']/lst[@name='suggestions']/lst[@name='ac']/ int [@name='numFound'][.='2'] [junit] xml response was: <?xml version= "1.0" encoding= "UTF-8" ?> [junit] <response> [junit] <lst name= "responseHeader" >< int name= "status" >0</ int >< int name= "QTime" >1</ int ></lst><lst name= "spellcheck" ><lst name= "suggestions" /></lst> [junit] </response> Seems like the dict isn't being built for some reason, but it always seems to pass if I run the test individually. I'll try and reproduce with logging at INFO.
          Hide
          Steve Rowe added a comment -

          SuggesterTest fails quite a bit for me...

          I've seen the same thing on and off, but just like your case, when I run it individually, it succeeds.

          Show
          Steve Rowe added a comment - SuggesterTest fails quite a bit for me... I've seen the same thing on and off, but just like your case, when I run it individually, it succeeds.
          Hide
          Robert Muir added a comment -

          Is this a race condition?

          If i look at suggesters code i notice:
          in build()

              lookup = (Lookup) core.getResourceLoader().newInstance(lookupImpl);
              try {
                lookup.build(dictionary);
          

          in getSuggestions()

              if (lookup == null) {
                LOG.info("Lookup is null - invoke spellchecker.build first");
          

          seems like lookup could be non-null, but just not contain any items and cause the test to fail without the warning...

          so maybe the issue happens only if your machine is very busy (e.g. when running all the tests and there are several jvms going) ?

          Show
          Robert Muir added a comment - Is this a race condition? If i look at suggesters code i notice: in build() lookup = (Lookup) core.getResourceLoader().newInstance(lookupImpl); try { lookup.build(dictionary); in getSuggestions() if (lookup == null ) { LOG.info( "Lookup is null - invoke spellchecker.build first" ); seems like lookup could be non-null, but just not contain any items and cause the test to fail without the warning... so maybe the issue happens only if your machine is very busy (e.g. when running all the tests and there are several jvms going) ?
          Hide
          Yonik Seeley added a comment -

          Hmmm, when I try changing the "SEVERE" to "INFO" in solr/testlogging.properties, it only seems to take effect on a few tests.
          This worked fine in the past... any ideas?

          Show
          Yonik Seeley added a comment - Hmmm, when I try changing the "SEVERE" to "INFO" in solr/testlogging.properties, it only seems to take effect on a few tests. This worked fine in the past... any ideas?
          Hide
          Robert Muir added a comment -

          Yonik, do you have a specific test it doesnt work for? it spews a ton of INFO out for me!

          Show
          Robert Muir added a comment - Yonik, do you have a specific test it doesnt work for? it spews a ton of INFO out for me!
          Hide
          Yonik Seeley added a comment -

          Most of them I think... only the first 10 tests on this list have INFO debugging:

          $ ls -lS | head -20
          total 1380
          -rwxr-xr-x  1 yonik None 449054 Oct 13 11:47 TEST-org.apache.solr.ConvertedLegacyTest.txt
          -rwxr-xr-x  1 yonik None 218969 Oct 13 11:47 TEST-org.apache.solr.BasicFunctionalityTest.txt
          -rwxr-xr-x  1 yonik None 158532 Oct 13 11:50 TEST-org.apache.solr.handler.ExtractingRequestHandlerTest.txt
          -rwxr-xr-x  1 yonik None 104678 Oct 13 11:46 TEST-org.apache.solr.client.solrj.embedded.LargeVolumeJettyTest.txt
          -rwxr-xr-x  1 yonik None  56895 Oct 13 11:45 TEST-org.apache.solr.DisMaxRequestHandlerTest.txt
          -rwxr-xr-x  1 yonik None  25873 Oct 13 11:49 TEST-org.apache.solr.handler.clustering.ClusteringComponentTest.txt
          -rwxr-xr-x  1 yonik None  13901 Oct 13 11:45 TEST-org.apache.solr.EchoParamsTest.txt
          -rwxr-xr-x  1 yonik None  13577 Oct 13 11:47 TEST-org.apache.solr.OutputWriterTest.txt
          -rwxr-xr-x  1 yonik None   9498 Oct 13 11:45 TEST-org.apache.solr.TestPluginEnable.txt
          -rwxr-xr-x  1 yonik None   9283 Oct 13 11:45 TEST-org.apache.solr.TestSolrCoreProperties.txt
          drwxr-xr-x+ 1 yonik None   4096 Oct 13 11:50 temp
          -rwxr-xr-x  1 yonik None   2669 Oct 13 11:49 TEST-org.apache.solr.handler.dataimport.TestLineEntityProcessor.txt
          -rwxr-xr-x  1 yonik None   1896 Oct 13 11:46 TEST-org.apache.solr.handler.component.DistributedSpellCollatorTest.txt
          -rwxr-xr-x  1 yonik None   1544 Oct 13 11:49 TEST-org.apache.solr.handler.dataimport.TestCachedSqlEntityProcessor.txt
          -rwxr-xr-x  1 yonik None   1337 Oct 13 11:47 TEST-org.apache.solr.request.SimpleFacetsTest.txt
          -rwxr-xr-x  1 yonik None   1291 Oct 13 11:46 TEST-org.apache.solr.highlight.HighlighterTest.txt
          -rwxr-xr-x  1 yonik None   1211 Oct 13 11:48 TEST-org.apache.solr.handler.component.TermsComponentTest.txt
          -rwxr-xr-x  1 yonik None   1168 Oct 13 11:46 TEST-org.apache.solr.handler.TestReplicationHandler.txt
          -rwxr-xr-x  1 yonik None   1119 Oct 13 11:48 TEST-org.apache.solr.handler.component.SpellCheckComponentTest.txt
          
          Show
          Yonik Seeley added a comment - Most of them I think... only the first 10 tests on this list have INFO debugging: $ ls -lS | head -20 total 1380 -rwxr-xr-x 1 yonik None 449054 Oct 13 11:47 TEST-org.apache.solr.ConvertedLegacyTest.txt -rwxr-xr-x 1 yonik None 218969 Oct 13 11:47 TEST-org.apache.solr.BasicFunctionalityTest.txt -rwxr-xr-x 1 yonik None 158532 Oct 13 11:50 TEST-org.apache.solr.handler.ExtractingRequestHandlerTest.txt -rwxr-xr-x 1 yonik None 104678 Oct 13 11:46 TEST-org.apache.solr.client.solrj.embedded.LargeVolumeJettyTest.txt -rwxr-xr-x 1 yonik None 56895 Oct 13 11:45 TEST-org.apache.solr.DisMaxRequestHandlerTest.txt -rwxr-xr-x 1 yonik None 25873 Oct 13 11:49 TEST-org.apache.solr.handler.clustering.ClusteringComponentTest.txt -rwxr-xr-x 1 yonik None 13901 Oct 13 11:45 TEST-org.apache.solr.EchoParamsTest.txt -rwxr-xr-x 1 yonik None 13577 Oct 13 11:47 TEST-org.apache.solr.OutputWriterTest.txt -rwxr-xr-x 1 yonik None 9498 Oct 13 11:45 TEST-org.apache.solr.TestPluginEnable.txt -rwxr-xr-x 1 yonik None 9283 Oct 13 11:45 TEST-org.apache.solr.TestSolrCoreProperties.txt drwxr-xr-x+ 1 yonik None 4096 Oct 13 11:50 temp -rwxr-xr-x 1 yonik None 2669 Oct 13 11:49 TEST-org.apache.solr.handler.dataimport.TestLineEntityProcessor.txt -rwxr-xr-x 1 yonik None 1896 Oct 13 11:46 TEST-org.apache.solr.handler.component.DistributedSpellCollatorTest.txt -rwxr-xr-x 1 yonik None 1544 Oct 13 11:49 TEST-org.apache.solr.handler.dataimport.TestCachedSqlEntityProcessor.txt -rwxr-xr-x 1 yonik None 1337 Oct 13 11:47 TEST-org.apache.solr.request.SimpleFacetsTest.txt -rwxr-xr-x 1 yonik None 1291 Oct 13 11:46 TEST-org.apache.solr.highlight.HighlighterTest.txt -rwxr-xr-x 1 yonik None 1211 Oct 13 11:48 TEST-org.apache.solr.handler.component.TermsComponentTest.txt -rwxr-xr-x 1 yonik None 1168 Oct 13 11:46 TEST-org.apache.solr.handler.TestReplicationHandler.txt -rwxr-xr-x 1 yonik None 1119 Oct 13 11:48 TEST-org.apache.solr.handler.component.SpellCheckComponentTest.txt
          Hide
          Yonik Seeley added a comment -

          If I run a test individually, it does have all of the requested logging... it's just when I run everything that the logging is missing.

          Show
          Yonik Seeley added a comment - If I run a test individually, it does have all of the requested logging... it's just when I run everything that the logging is missing.
          Hide
          Robert Muir added a comment -

          I see your problem here too... but i have no ideas at the moment.

          Show
          Robert Muir added a comment - I see your problem here too... but i have no ideas at the moment.
          Hide
          Robert Muir added a comment -

          If I run a test individually, it does have all of the requested logging... it's just when I run everything that the logging is missing.

          I broke this when i changed the build not to fork a new JVM for each test class.
          You can see this easier if you use -Dtests.threadspercpu=0 (run all tests in one single jvm, no parallel tests). Then you will
          see the logging only working for the very first test class (BasicFunctionalityTest)

          I don't understand why the logging shuts up in this case (maybe something in SolrTestCase4J is "shutting down" the logging in afterClass?)
          Sorry I get confused really quickly with these logging frameworks, so I don't even know where to begin.

          Show
          Robert Muir added a comment - If I run a test individually, it does have all of the requested logging... it's just when I run everything that the logging is missing. I broke this when i changed the build not to fork a new JVM for each test class. You can see this easier if you use -Dtests.threadspercpu=0 (run all tests in one single jvm, no parallel tests). Then you will see the logging only working for the very first test class (BasicFunctionalityTest) I don't understand why the logging shuts up in this case (maybe something in SolrTestCase4J is "shutting down" the logging in afterClass?) Sorry I get confused really quickly with these logging frameworks, so I don't even know where to begin.
          Hide
          Yonik Seeley added a comment -

          I just tried peppering SolrTestCaseJ4 with checkLogging calls (just checks that INFO is still enabled for the log object there).
          It never tripped (i.e. log.isInfoEnabled() is true for the entire "ant test" run).

          Then you will see the logging only working for the very first test class (BasicFunctionalityTest)

          OK, good to know - so it's not something like Jetty shutting down the logging somehow.
          AFAIK, SLF4J doesn't even have any mechanism to shut down logging. Nothing I know of in Solr messes with the logging (but I don't know much about logging frameworks either).

          Show
          Yonik Seeley added a comment - I just tried peppering SolrTestCaseJ4 with checkLogging calls (just checks that INFO is still enabled for the log object there). It never tripped (i.e. log.isInfoEnabled() is true for the entire "ant test" run). Then you will see the logging only working for the very first test class (BasicFunctionalityTest) OK, good to know - so it's not something like Jetty shutting down the logging somehow. AFAIK, SLF4J doesn't even have any mechanism to shut down logging. Nothing I know of in Solr messes with the logging (but I don't know much about logging frameworks either).
          Hide
          Hoss Man added a comment -

          Two things i've noticed recently that are kind of odd (and per robert on IRC, weren't intentional)

          1) all of the "temp" files created by tests are now getting created in the same output directory as the test results (ie:lucene/build/test and solr/build/test-results) ...

          hossman@bester:~/lucene/dev$ ls lucene/build/test/ | head
          1
          2
          3
          4
          5
          6
          7
          8
          TEST-org.apache.lucene.analysis.TestCachingTokenFilter.xml
          TEST-org.apache.lucene.analysis.TestCharTokenizers.xml
          hossman@bester:~/lucene/dev$ ls lucene/build/test/1 | head
          quiet.ant
          test1427325378064240227tmp
          test2206717657854845874tmp
          test2349170826556141314tmp
          test2474659255857912171tmp
          test2655069444325547827tmp
          test3737548537987799034tmp
          test5395869393791701784tmp
          test5824921577206577644tmp
          test5897381366443436559tmp
          

          ...these temp dirs should probably go under some other parent dir (either java.io.tmpdir or some place else in "build") so that it's eaiser to search the test result files w/o getting stray matches on the tmp files (in particular: i know that the hudson junit plugin and the ant <junitreport /> tasks both walk every file in those directories, so we should minimize the amount of extra stuff they have to look at.

          2) the Solr tests now seem to be formatted using the "plain text" formatter by default, and not the xml formatter...

          hossman@bester:~/lucene/dev$ ls solr/build/test-results/ | head -5
          temp
          TEST-org.apache.solr.analysis.EnglishPorterFilterFactoryTest.txt
          TEST-org.apache.solr.analysis.TestBrazilianStemFilterFactory.txt
          TEST-org.apache.solr.analysis.TestBufferedTokenStream.txt
          TEST-org.apache.solr.analysis.TestCapitalizationFilterFactory.txt
          

          ..using plain text was always an option before, but now it seems to be the default

          Show
          Hoss Man added a comment - Two things i've noticed recently that are kind of odd (and per robert on IRC, weren't intentional) 1) all of the "temp" files created by tests are now getting created in the same output directory as the test results (ie:lucene/build/test and solr/build/test-results) ... hossman@bester:~/lucene/dev$ ls lucene/build/test/ | head 1 2 3 4 5 6 7 8 TEST-org.apache.lucene.analysis.TestCachingTokenFilter.xml TEST-org.apache.lucene.analysis.TestCharTokenizers.xml hossman@bester:~/lucene/dev$ ls lucene/build/test/1 | head quiet.ant test1427325378064240227tmp test2206717657854845874tmp test2349170826556141314tmp test2474659255857912171tmp test2655069444325547827tmp test3737548537987799034tmp test5395869393791701784tmp test5824921577206577644tmp test5897381366443436559tmp ...these temp dirs should probably go under some other parent dir (either java.io.tmpdir or some place else in "build") so that it's eaiser to search the test result files w/o getting stray matches on the tmp files (in particular: i know that the hudson junit plugin and the ant <junitreport /> tasks both walk every file in those directories, so we should minimize the amount of extra stuff they have to look at. 2) the Solr tests now seem to be formatted using the "plain text" formatter by default, and not the xml formatter... hossman@bester:~/lucene/dev$ ls solr/build/test-results/ | head -5 temp TEST-org.apache.solr.analysis.EnglishPorterFilterFactoryTest.txt TEST-org.apache.solr.analysis.TestBrazilianStemFilterFactory.txt TEST-org.apache.solr.analysis.TestBufferedTokenStream.txt TEST-org.apache.solr.analysis.TestCapitalizationFilterFactory.txt ..using plain text was always an option before, but now it seems to be the default
          Hide
          Hoss Man added a comment -

          this patch gets the solr tests using the xml formatter by default .. but looking at the history it seems like yonik deliberately switched solr to use plain text by default back before Solr 1.4 (r819413) ... evidently i had some property override somewhere that i've recently lost.

          i'm not sure why yonik thought plain text was better by default, but i mainly just think we should be consistent (right now solr is plain text by default, but the rest of hte tree is xml by default)

          Show
          Hoss Man added a comment - this patch gets the solr tests using the xml formatter by default .. but looking at the history it seems like yonik deliberately switched solr to use plain text by default back before Solr 1.4 (r819413) ... evidently i had some property override somewhere that i've recently lost. i'm not sure why yonik thought plain text was better by default, but i mainly just think we should be consistent (right now solr is plain text by default, but the rest of hte tree is xml by default)
          Hide
          Yonik Seeley added a comment -

          The text format is so much easier to read, and it also streams correctly. The XML impl buffers everything in memory, and even leads to OOM errors if you try a long-running test.

          Show
          Yonik Seeley added a comment - The text format is so much easier to read, and it also streams correctly. The XML impl buffers everything in memory, and even leads to OOM errors if you try a long-running test.
          Hide
          Robert Muir added a comment -

          heres a patch for a pet peeve of mine, if you run parallel tests (say 8 jvms) and lets say 8 tests fail, one in each, you get this annoying

          BUILD FAILED
          BUILD FAILED
          BUILD FAILED
          ...

          patch changes it to only print once.

          Show
          Robert Muir added a comment - heres a patch for a pet peeve of mine, if you run parallel tests (say 8 jvms) and lets say 8 tests fail, one in each, you get this annoying BUILD FAILED BUILD FAILED BUILD FAILED ... patch changes it to only print once.
          Hide
          Fuad Efendi added a comment -

          Words such as "xml formatter" sound extremely funny for me... why can't it be UTF8 formatter? Or Base64 for instance, JSON, EDI? Formatted...

          Show
          Fuad Efendi added a comment - Words such as "xml formatter" sound extremely funny for me... why can't it be UTF8 formatter? Or Base64 for instance, JSON, EDI? Formatted...
          Hide
          Hoss Man added a comment -

          Words such as "xml formatter" sound extremely funny for me... why can't it be UTF8 formatter? Or Base64 for instance, JSON, EDI? Formatted...

          "formatter" has a very specific meaning in the ant junit taskdef. "xml" is a legal "type" for the <formatter/>

          UTF8, Base64, JSON, and EDI are not.

          Show
          Hoss Man added a comment - Words such as "xml formatter" sound extremely funny for me... why can't it be UTF8 formatter? Or Base64 for instance, JSON, EDI? Formatted... "formatter" has a very specific meaning in the ant junit taskdef. "xml" is a legal "type" for the <formatter/> UTF8, Base64, JSON, and EDI are not.
          Hide
          Hoss Man added a comment -

          The text format is so much easier to read, and it also streams correctly. The XML impl buffers everything in memory,

          and even leads to OOM errors if you try a long-running test.

          Ah .. ok .. i remember this being discussed now. (we still have to be conciencious about the OOM issue though since hudson requires the xml formatter)

          If "plain" makes sense for Solr because of readability, streaming, and OOMs, then shouldn't it be the default for the entire tree?

          Show
          Hoss Man added a comment - The text format is so much easier to read, and it also streams correctly. The XML impl buffers everything in memory, and even leads to OOM errors if you try a long-running test. Ah .. ok .. i remember this being discussed now. (we still have to be conciencious about the OOM issue though since hudson requires the xml formatter) If "plain" makes sense for Solr because of readability, streaming, and OOMs, then shouldn't it be the default for the entire tree?
          Hide
          Robert Muir added a comment -

          Ah .. ok .. i remember this being discussed now. (we still have to be conciencious about the OOM issue though since hudson requires the xml formatter)

          If "plain" makes sense for Solr because of readability, streaming, and OOMs, then shouldn't it be the default for the entire tree?

          sounds good to me, sounds like we should default lucene's build to plain but let it take a -D for hudson?

          Show
          Robert Muir added a comment - Ah .. ok .. i remember this being discussed now. (we still have to be conciencious about the OOM issue though since hudson requires the xml formatter) If "plain" makes sense for Solr because of readability, streaming, and OOMs, then shouldn't it be the default for the entire tree? sounds good to me, sounds like we should default lucene's build to plain but let it take a -D for hudson?
          Hide
          Yonik Seeley added a comment -

          Here's the logs from a failed run of SuggesterTest

          Show
          Yonik Seeley added a comment - Here's the logs from a failed run of SuggesterTest
          Hide
          Yonik Seeley added a comment -

          The SpellCheckerListener code looks strange... reload() if firstSearcher, build if newSearcher?
          Does reload() imply build()? If so, it's a suggester problem, if not, it's a SpellCheckerListener problem.

          Show
          Yonik Seeley added a comment - The SpellCheckerListener code looks strange... reload() if firstSearcher, build if newSearcher? Does reload() imply build()? If so, it's a suggester problem, if not, it's a SpellCheckerListener problem.
          Hide
          Andrzej Bialecki added a comment -

          Does reload() imply build()? If so, it's a suggester problem, if not, it's a SpellCheckerListener problem.

          The problem is in Suggester, and it's related to a non-persistent implementation of Lookup in Suggester, as opposed to a persistent spellchecking index. With spellchecker, when firstSearcher event is processed we want to load an existing spellchecking index (that's why it doesn't call build(), but reload()). On the other hand, if we want to rebuild the spellchecking index on commit, and we process a newSearcher event, then we need to use build(), not reload().

          So far the code was correct. However, current implementations of Lookup don't persist their data, so it's lost after commit and it needs to be rebuilt both on firstSearcher and on newSearcher. I created SOLR-2173 to track this issue.

          Show
          Andrzej Bialecki added a comment - Does reload() imply build()? If so, it's a suggester problem, if not, it's a SpellCheckerListener problem. The problem is in Suggester, and it's related to a non-persistent implementation of Lookup in Suggester, as opposed to a persistent spellchecking index. With spellchecker, when firstSearcher event is processed we want to load an existing spellchecking index (that's why it doesn't call build(), but reload()). On the other hand, if we want to rebuild the spellchecking index on commit, and we process a newSearcher event, then we need to use build(), not reload(). So far the code was correct. However, current implementations of Lookup don't persist their data, so it's lost after commit and it needs to be rebuilt both on firstSearcher and on newSearcher. I created SOLR-2173 to track this issue.
          Hide
          Robert Muir added a comment -

          I think we had a really good start here at one point, but the problem is that the codebase moves too quickly and we didnt get time to wrap it up.

          I think its really important to cleanup the build here so that e.g. the Solr build extends lucene's like the patch started, with all targets being recursive so you can easily add new functionality (e.g. findbugs) in a single place.

          Maybe ill create a svn branch to get this done, seems insane to create a branch for the build system but this way it might actually get done instead of just becoming outdated patches.

          Show
          Robert Muir added a comment - I think we had a really good start here at one point, but the problem is that the codebase moves too quickly and we didnt get time to wrap it up. I think its really important to cleanup the build here so that e.g. the Solr build extends lucene's like the patch started, with all targets being recursive so you can easily add new functionality (e.g. findbugs) in a single place. Maybe ill create a svn branch to get this done, seems insane to create a branch for the build system but this way it might actually get done instead of just becoming outdated patches.
          Hide
          Chris Male added a comment -

          Hey Robert, I'd like to chip in here where I can. What further improvements do you have in mind?

          Show
          Chris Male added a comment - Hey Robert, I'd like to chip in here where I can. What further improvements do you have in mind?
          Hide
          Robert Muir added a comment -

          Hi Chris, so mainly i want to start with the patch where Solr's build system extends Lucene's.

          This gives us a 'single' build system (as the modules and lucene contrib already extend lucene's) rather than the 4 or 5 we have now: as solr contribs actually basically have their own standalone build.

          So in lucene's common-build, we can easily add new targets/checks/functionality (e.g. findbugs/failing on javadocs/etc) and it applies to everything.

          Also in my tests this was a huge speedup to the solr build, especially things like ant test -Dtestcase

          But, there is a lot to do: in my patch i only did the basics (compile/test/etc).

          • we need to implement things like solr 'example' tasks.
          • we need to look at lucene and solr's build.xml and see what things are duplicates, such as packaging tasks, that can be added to common-build.xml: an example is artifact signing and hashing.
          • i think it might be worth our time to look at antunit or something of that nature to implement 'tests' for the build system. By this i don't really mean tests that ensure our ant logic is correct: I mean automated tests that look at whether things like packaging are correct. But i havent looked at antunit yet, so I admittedly don't know what I am talking about.
          • once everything is using a single common-build recursively, we should consider additional features (pmd, findbugs, ...) that we would want to implement to perform even more checks: this can really be separate issues but its good to think right now about how we can make our build system as picky as possible to try to keep the codebase in good shape.
          Show
          Robert Muir added a comment - Hi Chris, so mainly i want to start with the patch where Solr's build system extends Lucene's. This gives us a 'single' build system (as the modules and lucene contrib already extend lucene's) rather than the 4 or 5 we have now: as solr contribs actually basically have their own standalone build. So in lucene's common-build, we can easily add new targets/checks/functionality (e.g. findbugs/failing on javadocs/etc) and it applies to everything. Also in my tests this was a huge speedup to the solr build, especially things like ant test -Dtestcase But, there is a lot to do: in my patch i only did the basics (compile/test/etc). we need to implement things like solr 'example' tasks. we need to look at lucene and solr's build.xml and see what things are duplicates, such as packaging tasks, that can be added to common-build.xml: an example is artifact signing and hashing. i think it might be worth our time to look at antunit or something of that nature to implement 'tests' for the build system. By this i don't really mean tests that ensure our ant logic is correct: I mean automated tests that look at whether things like packaging are correct. But i havent looked at antunit yet, so I admittedly don't know what I am talking about. once everything is using a single common-build recursively, we should consider additional features (pmd, findbugs, ...) that we would want to implement to perform even more checks: this can really be separate issues but its good to think right now about how we can make our build system as picky as possible to try to keep the codebase in good shape.
          Hide
          Chris Male added a comment -

          once everything is using a single common-build recursively, we should consider additional features (pmd, findbugs, ...) that we would want to implement to perform even more checks: this can really be separate issues but its good to think right now about how we can make our build system as picky as possible to try to keep the codebase in good shape.

          Do you envisage these additional features being on a per module (by which I mean anything with a build.xml) basis, or more global in the common-build.xml?

          Show
          Chris Male added a comment - once everything is using a single common-build recursively, we should consider additional features (pmd, findbugs, ...) that we would want to implement to perform even more checks: this can really be separate issues but its good to think right now about how we can make our build system as picky as possible to try to keep the codebase in good shape. Do you envisage these additional features being on a per module (by which I mean anything with a build.xml) basis, or more global in the common-build.xml?
          Hide
          Robert Muir added a comment -

          definitely in the common-build. this way you implement a new 'feature' in a single place.

          Currently to do this you have to put things in multiple places, which discourages us from improving the build.

          Show
          Robert Muir added a comment - definitely in the common-build. this way you implement a new 'feature' in a single place. Currently to do this you have to put things in multiple places, which discourages us from improving the build.
          Hide
          Robert Muir added a comment -

          Bulk move 3.2 -> 3.3

          Show
          Robert Muir added a comment - Bulk move 3.2 -> 3.3
          Hide
          Robert Muir added a comment -

          3.4 -> 3.5

          Show
          Robert Muir added a comment - 3.4 -> 3.5
          Hide
          Steve Rowe added a comment -

          Robert, I think this can be resolved as fixed.

          Show
          Steve Rowe added a comment - Robert, I think this can be resolved as fixed.

            People

            • Assignee:
              Robert Muir
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development