Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9065

Migrate solrj tests from AbstractDistribZkTestBase to SolrCloudTestCase

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: 6.0, 6.1
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      AbstractDistribZkTestBase sets up collections using the legacy core-based system, and does a lot of comparing things against control collections that the SolrJ tests really don't require. We should migrate these tests to using SolrCloudTestCase instead.

      1. SOLR-9065.patch
        362 kB
        Alan Woodward

        Issue Links

          Activity

          Hide
          romseygeek Alan Woodward added a comment -

          This patch migrates the following test classes:

          • solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
          • solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java
          • solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java
          • solr/solrj/src/test/org/apache/solr/client/solrj/io/sql/JdbcTest.java
          • solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/JDBCStreamTest.java
          • solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionTest.java
          • solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java

          Running 'ant test -Dtests.slow=true' in solrj goes from 5 mins before this patch to 2mins 20seconds afterwards on my machine.

          It also includes a couple of small usability changes to UpdateRequest and CollectionAdminRequest.

          Show
          romseygeek Alan Woodward added a comment - This patch migrates the following test classes: solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/sql/JdbcTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/JDBCStreamTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java Running 'ant test -Dtests.slow=true' in solrj goes from 5 mins before this patch to 2mins 20seconds afterwards on my machine. It also includes a couple of small usability changes to UpdateRequest and CollectionAdminRequest.
          Hide
          hossman Hoss Man added a comment -

          +1

          In general, it makes sense to have some tests still do the "comparing things against (single core) control collections" but most tests don't need it, and starting with these tests is a great start.

          Show
          hossman Hoss Man added a comment - +1 In general, it makes sense to have some tests still do the "comparing things against (single core) control collections" but most tests don't need it, and starting with these tests is a great start.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 630a8c950d89064b7f2e8dbe865f964a21f9f501 in lucene-solr's branch refs/heads/master from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=630a8c9 ]

          SOLR-9065: Migrate SolrJ tests to SolrCloudTestCase

          Show
          jira-bot ASF subversion and git services added a comment - Commit 630a8c950d89064b7f2e8dbe865f964a21f9f501 in lucene-solr's branch refs/heads/master from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=630a8c9 ] SOLR-9065 : Migrate SolrJ tests to SolrCloudTestCase
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7a8d4947a5f85f8eb7c6103292d2f5e9a8112ede in lucene-solr's branch refs/heads/branch_6x from Alan Woodward
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7a8d494 ]

          SOLR-9065: Migrate SolrJ tests to SolrCloudTestCase

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7a8d4947a5f85f8eb7c6103292d2f5e9a8112ede in lucene-solr's branch refs/heads/branch_6x from Alan Woodward [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7a8d494 ] SOLR-9065 : Migrate SolrJ tests to SolrCloudTestCase
          Hide
          romseygeek Alan Woodward added a comment -

          Thanks Hoss

          Show
          romseygeek Alan Woodward added a comment - Thanks Hoss
          Hide
          joel.bernstein Joel Bernstein added a comment - - edited

          Can we revert this ticket? I have a bunch of test cases ready to go for the 6.1 release for the new Graph traversal features. This has blindsided me completely.

          Show
          joel.bernstein Joel Bernstein added a comment - - edited Can we revert this ticket? I have a bunch of test cases ready to go for the 6.1 release for the new Graph traversal features. This has blindsided me completely.
          Hide
          joel.bernstein Joel Bernstein added a comment - - edited

          A large patch went up yesterday for this ticket. Then it's committed very quickly without consulting the people who are working with these test cases.

          Show
          joel.bernstein Joel Bernstein added a comment - - edited A large patch went up yesterday for this ticket. Then it's committed very quickly without consulting the people who are working with these test cases.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          I'm going to put the old version of the GraphExpressionTest back in place so I can move forward.

          Show
          joel.bernstein Joel Bernstein added a comment - I'm going to put the old version of the GraphExpressionTest back in place so I can move forward.
          Hide
          erickerickson Erick Erickson added a comment -

          The wholesale reworking of the tests is making keeping 8467 up to date "interesting". Do we have a good read on when 6.1 will be coming out? If it's very far in the future I'd like to put 8467 to bed.

          Show
          erickerickson Erick Erickson added a comment - The wholesale reworking of the tests is making keeping 8467 up to date "interesting". Do we have a good read on when 6.1 will be coming out? If it's very far in the future I'd like to put 8467 to bed.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          The wholesale reworking of tests needs to be coordinated with the people who wrote the original tests and maintain those tests.

          Show
          joel.bernstein Joel Bernstein added a comment - The wholesale reworking of tests needs to be coordinated with the people who wrote the original tests and maintain those tests.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          The following test cases were clearly under development at the time when this ticket was committed:

          solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java
          solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java
          solr/solrj/src/test/org/apache/solr/client/solrj/io/sql/JdbcTest.java
          solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/JDBCStreamTest.java
          solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionTest.java
          solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java

          All that needed to be done was to notify the committers who maintain these tests and let them know a wholesale change was coming. Then there could have been a conversation about how to move forward without stepping on peoples work. Instead a patch went up and the next day it was committed.

          Show
          joel.bernstein Joel Bernstein added a comment - The following test cases were clearly under development at the time when this ticket was committed: solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphExpressionTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/graph/GraphTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/sql/JdbcTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/JDBCStreamTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamExpressionTest.java solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamingTest.java All that needed to be done was to notify the committers who maintain these tests and let them know a wholesale change was coming. Then there could have been a conversation about how to move forward without stepping on peoples work. Instead a patch went up and the next day it was committed.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          We have a commit then review policy and generally favor forward progress.

          I appreciate that Alan's work here has caused some concern around other work and 6.1. So what? Alan's work was fine and had multiple eyes on it. Now there are more eyes and Alan will be open to working with them as he always is.

          If one of the tests that was converted needs to be un-converted or briefly un-converted, so what? Perhaps there are other short term options as well. It's a large code base and many issues cross a lot of code. We will step on each other's toes, it's part of the game.

          Show
          markrmiller@gmail.com Mark Miller added a comment - We have a commit then review policy and generally favor forward progress. I appreciate that Alan's work here has caused some concern around other work and 6.1. So what? Alan's work was fine and had multiple eyes on it. Now there are more eyes and Alan will be open to working with them as he always is. If one of the tests that was converted needs to be un-converted or briefly un-converted, so what? Perhaps there are other short term options as well. It's a large code base and many issues cross a lot of code. We will step on each other's toes, it's part of the game.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          Yeah, but this was a wholesale change, committed one day after the patch went up.

          It's not hard to some have consideration for other peoples work. A simple heads up will suffice.

          Show
          joel.bernstein Joel Bernstein added a comment - Yeah, but this was a wholesale change, committed one day after the patch went up. It's not hard to some have consideration for other peoples work. A simple heads up will suffice.
          Hide
          markrmiller@gmail.com Mark Miller added a comment - - edited

          There was a heads up on this issue. 9 times out of 10, this would have caused no concern. These kind of changes are made all the time. If we had to reach out to each person that might give a damn and wait a week for every change, we would slow down a little.

          It's commit then review. Alan committed, now you are reviewing. That is how we do it.

          Show
          markrmiller@gmail.com Mark Miller added a comment - - edited There was a heads up on this issue. 9 times out of 10, this would have caused no concern. These kind of changes are made all the time. If we had to reach out to each person that might give a damn and wait a week for every change, we would slow down a little. It's commit then review. Alan committed, now you are reviewing. That is how we do it.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          But this patch had 7000 lines of code and it went up yesterday. I think that's too fast.

          Show
          joel.bernstein Joel Bernstein added a comment - But this patch had 7000 lines of code and it went up yesterday. I think that's too fast.
          Hide
          dsmiley David Smiley added a comment -

          I agree with Joel. And I don't think there's any hard feelings, just a "shout out" as Joel said would have been great. Alan Woodward can you please send a message to the dev list to announce your refactoring plans/schedule so help us coordinate our efforts? That would be very helpful – thanks in advance.

          Show
          dsmiley David Smiley added a comment - I agree with Joel. And I don't think there's any hard feelings, just a "shout out" as Joel said would have been great. Alan Woodward can you please send a message to the dev list to announce your refactoring plans/schedule so help us coordinate our efforts? That would be very helpful – thanks in advance.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          There are no hard feelings. I think the patch looks good. I've already re-worked what i needed to in the GraphExpressionTest.

          Show
          joel.bernstein Joel Bernstein added a comment - There are no hard feelings. I think the patch looks good. I've already re-worked what i needed to in the GraphExpressionTest.
          Hide
          markrmiller@gmail.com Mark Miller added a comment - - edited

          just a "shout out"

          Follow the mailing list. We are not a review then commit project. The result that happened is how this should work. Then 9 out of 10 times we move fast, and once we make an adjustment to the commit.

          Everyone just needs to relax. No one owns any area of the code, no one has to be checked with before changes. It's on anyone who cares to follow email and JIRA. This did not appear to be a controversial change, a patch went up, hossman +1'd.

          This is how it's all supposed to work.

          Show
          markrmiller@gmail.com Mark Miller added a comment - - edited just a "shout out" Follow the mailing list. We are not a review then commit project. The result that happened is how this should work. Then 9 out of 10 times we move fast, and once we make an adjustment to the commit. Everyone just needs to relax. No one owns any area of the code, no one has to be checked with before changes. It's on anyone who cares to follow email and JIRA. This did not appear to be a controversial change, a patch went up, hossman +1'd. This is how it's all supposed to work.
          Hide
          dsmiley David Smiley added a comment -

          I think there may be a misunderstanding of what Joel and I mean by a "shout out". We don't mean a review of a patch (yes, that would then be RTC). We mean a helpful and courteous notice to the dev list (not a comment on an issue). This is very much like what Hoss is doing with the whole JIRA master/6.0 versioning issue. Both this issue and that one have the potential for a wide impact which is why we suggest a "shout out" first. I'm relaxed; I hope you feel relaxed too and Alan and Joel

          Show
          dsmiley David Smiley added a comment - I think there may be a misunderstanding of what Joel and I mean by a "shout out". We don't mean a review of a patch (yes, that would then be RTC). We mean a helpful and courteous notice to the dev list (not a comment on an issue). This is very much like what Hoss is doing with the whole JIRA master/6.0 versioning issue. Both this issue and that one have the potential for a wide impact which is why we suggest a "shout out" first. I'm relaxed; I hope you feel relaxed too and Alan and Joel
          Hide
          romseygeek Alan Woodward added a comment -

          can you please send a message to the dev list to announce your refactoring plans/schedule so help us coordinate our efforts?

          Will do so in the next couple of days. Sorry for stepping on your toes, guys.

          Show
          romseygeek Alan Woodward added a comment - can you please send a message to the dev list to announce your refactoring plans/schedule so help us coordinate our efforts? Will do so in the next couple of days. Sorry for stepping on your toes, guys.
          Hide
          joel.bernstein Joel Bernstein added a comment -

          No problem, and thanks for your work on the tests. It's a big improvement!

          Show
          joel.bernstein Joel Bernstein added a comment - No problem, and thanks for your work on the tests. It's a big improvement!
          Hide
          mkhludnev Mikhail Khludnev added a comment -

          Alan Woodward, can you consult me about distributed search testing? There is BaseDistributedSearchTestCase subclass in SOLR-5725.patch. It doesn't need Zookeeper, certainly, it's quite beneficial from _ "comparing things against (single core) control collections"_. Is it worth to move it to SolrCloudTestCase??

          Show
          mkhludnev Mikhail Khludnev added a comment - Alan Woodward , can you consult me about distributed search testing? There is BaseDistributedSearchTestCase subclass in SOLR-5725 .patch. It doesn't need Zookeeper, certainly, it's quite beneficial from _ "comparing things against (single core) control collections"_. Is it worth to move it to SolrCloudTestCase ??
          Hide
          romseygeek Alan Woodward added a comment -

          Hi Mikhail, it sounds as though for that case you don't need SolrCloudTestCase - it's only for situations where you want to set up a Cloud-mode cluster, ie one where you need Zookeeper for cluster management. If you're just testing single-core vs distributed, then BaseDistributedSearchTestCase is fine.

          Show
          romseygeek Alan Woodward added a comment - Hi Mikhail, it sounds as though for that case you don't need SolrCloudTestCase - it's only for situations where you want to set up a Cloud-mode cluster, ie one where you need Zookeeper for cluster management. If you're just testing single-core vs distributed, then BaseDistributedSearchTestCase is fine.

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              romseygeek Alan Woodward
            • Votes:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development