Details

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

      Description

      Tests can be split into categories.

      High-level categories:

      • Fast
      • Slow

      Low-level categories:

      • Vector
      • WebUI
      • Planner
      • Operator
      • Storage
      • Hive
      • JDBC
      • Kudu
      • Mongo
      • Hbase

      After the tests are categorized the Travis build can just run the fast tests.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user ilooner opened a pull request:

          https://github.com/apache/drill/pull/940

          DRILL-5752 Speed Up Unit Tests add Test Categories

          This PR does the following.

          *Increased Concurrency:* Previously tests only executed in 2 simultaneous processes, now one process per core is used to execute tests. This PR also fixed a few issues that popped up when concurrency was increased.

          *Test Categories:* Tests can now be executed according to their categories. There are two main categories of tests *Fast Tests* and *Secondary Tests. **Fast Tests* are fast and are executed by default with the following command:

          ```
          mvn -T 1C clean install
          ```

          *Note:* -T 1C increased the number of threads used to compile classes, and allows us to run tests for sub modules in parallel. This command takes about 15 minutes to run on my laptop.

          *Secondary Tests* are slower. To run both fast tests and *Secondary Tests* you can use the following command.

          ```
          mvn -T 1C clean install -DexcludedGroups=""
          ```

          In addition to *Fast* and *Secondary* tests there are more categories like:

          • *JdbcTest*
          • *ParquetTest*
          • *HiveStorageTest*
          • And many more

          All of these categories are in the *drill-common* sub modules in *org.apahce.drill.categories. All the tests are marked with one or more of these categories, and we can use these categories to select the tests we run. To run all the **Fast* tests that belong to the *JdbcTest* category we can use the following command:

          ```
          mvn -T 1C clean install -Dgroups="org.apache.drill.categories.JdbcTest"
          ```

          In order to run all the *Fast* AND *Secondary* JdbcTests you can use this command

          ```
          mvn -T 1C clean install -Dgroups="org.apache.drill.categories.JdbcTest" -DexcludedGroups=""
          ```

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/ilooner/drill DRILL-5752

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/drill/pull/940.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #940


          commit ba3d1ed53d7cabc5f5ac860531f6e989f567bc65
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-30T19:53:49Z

          • Removed redundant exclusion from java-exec pom
          • Marked the slow tests in contrib as secondary tests

          commit 9a3b7d2e1de85ce4397311e9ff2433432982fb03
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-30T22:02:23Z

          • Made slow unit tests SecondaryTests
          • Changed the number of forked test processes from just 2 processes to 1.5 process per core

          commit 4159bf8f3ad4966751b9176843e240bb4885cec5
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T00:10:38Z

          • Added JdbcTest category

          commit 0ab8ba8474b68bae8175f4b3c4a9b372f20eef8a
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T00:24:03Z

          • Removed unused imports from tests

          commit bf3748c6ddd96b303762d8fde9046215cb243c02
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T00:34:22Z

          • Added JdbcStorageTest Category

          commit 749fa9b1c4ae113a010de33bf359a38a1d56833e
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T18:15:18Z

          • Created the HiveStorageTest category
          • Made excludedGroups overrideable

          commit 50ddbb63f05eb08ac06d46dcf3c078e61dd3d042
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T18:39:28Z

          • Added HbaseStorageTest category

          commit 10097f61cb2df8898c2e7d8ff20cfb63fddcb6c5
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T18:50:33Z

          • Added the KuduStorageTest category

          commit 4dbad7edd5f479abc7db698e3ee2a2dad62abf20
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T19:06:34Z

          • Added the MongoStorageTest Category

          commit 5077b7beeccddcb0c84c70645c879cd3168f1bc1
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T19:42:09Z

          • Added a MemoryTest category

          commit a0e3832511041aeb18590d65ebb7166e40be045f
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T19:53:42Z

          • Add another secondary test.

          commit efc9b8bb5af3ce891b890217ed1595d0db26cbbb
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:03:08Z

          • Added a SecurityTest category
          • Made more of the security tests secondary tests

          commit 8dfacb0715252e063eca7d7a51895b8da191b273
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:05:05Z

          • Removed unused import for security test

          commit 72c3db7f1774fb0e37dd61a25ef1215f0391da63
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:05:57Z

          • Added another memory test

          commit 3fd24f8062887253a27bbdb8f31f12a06aa97270
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:22:14Z

          • Added an OperatorTest category

          commit 11290f9d48d223f8ca8b9c2df56e458616b51614
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:29:50Z

          • Added license headers
          • Added a ParquetTest category

          commit 3b03bb385466a2796578dd8ad96f2596a2b0f95e
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:33:35Z

          • Added a VectorTest category

          commit 4d242150cb8228ba044ad96706f09524664773bb
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:55:00Z

          • Added the SqlTest category

          commit 3cb3e895a9eb5171b88085c6984dbcb2685bbe58
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T20:59:14Z

          • Added more Vector tests

          commit 5f6ef77b65966b430a3b77ec577d3f2627133b59
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T21:01:07Z

          • Added more security tests

          commit 900911b2845e1a2b08d1f587afc505436ce6424e
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T21:03:26Z

          • Added another sql test

          commit 0fc54d716a9920bfd768d28314f9cdc30bf8d0d5
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T21:04:50Z

          • Added another security test

          commit 8e8c74b23646a3c6f203ae31468bf1e51787face
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T21:07:13Z

          • Added another vector test

          commit 0b0f42a85d327b5ef723aa4ba58b30a5854c8ca6
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T21:14:33Z

          • Added more operator and sql tests.

          commit f62ad15d3e0063032e55e857c898e08af7dc6084
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T21:39:01Z

          • Added the PlannerTest category
          • Categorized more tests

          commit 012e4a7b4048846e7ccca3b263c065daa1156d77
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T22:26:40Z

          • Added the SqlFunctionsTest category
          • Categorized more unit tests

          commit 04024ee2ae01d04070638e5e67f5dcc9cbc9305b
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-08-31T22:49:57Z

          • Fixed checkstyle errors

          commit c2d63579aaeac88ede9e369dee67db49a9595fc4
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-09-01T04:21:21Z

          • Moved test categories into their own package
          • Fixed race condition with jdbc storage plugin tests
          • Fixed prepared statement timeout with the drill-jdbc tests

          commit 47173b8616e5a08929f5a178206fbcb3431e1628
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-09-05T16:55:29Z

          • Cleaned up jdbc storage plugin pom
          • Increased the timeout of some unit tests
          • Increased the prepared statement creation timeout for some unit tests

          commit e0c9a361c7e089bec5713defbd41228369336325
          Author: Timothy Farkas <timothyfarkas@apache.org>
          Date: 2017-09-12T17:55:29Z

          • Decrease to one test process per core

          Show
          githubbot ASF GitHub Bot added a comment - GitHub user ilooner opened a pull request: https://github.com/apache/drill/pull/940 DRILL-5752 Speed Up Unit Tests add Test Categories This PR does the following. * Increased Concurrency: * Previously tests only executed in 2 simultaneous processes, now one process per core is used to execute tests. This PR also fixed a few issues that popped up when concurrency was increased. * Test Categories: * Tests can now be executed according to their categories. There are two main categories of tests * Fast Tests * and * Secondary Tests . **Fast Tests * are fast and are executed by default with the following command: ``` mvn -T 1C clean install ``` * Note: * -T 1C increased the number of threads used to compile classes, and allows us to run tests for sub modules in parallel. This command takes about 15 minutes to run on my laptop. * Secondary Tests * are slower. To run both fast tests and * Secondary Tests * you can use the following command. ``` mvn -T 1C clean install -DexcludedGroups="" ``` In addition to * Fast * and * Secondary * tests there are more categories like: * JdbcTest * * ParquetTest * * HiveStorageTest * And many more All of these categories are in the * drill-common * sub modules in * org.apahce.drill.categories . All the tests are marked with one or more of these categories, and we can use these categories to select the tests we run. To run all the **Fast * tests that belong to the * JdbcTest * category we can use the following command: ``` mvn -T 1C clean install -Dgroups="org.apache.drill.categories.JdbcTest" ``` In order to run all the * Fast * AND * Secondary * JdbcTests you can use this command ``` mvn -T 1C clean install -Dgroups="org.apache.drill.categories.JdbcTest" -DexcludedGroups="" ``` You can merge this pull request into a Git repository by running: $ git pull https://github.com/ilooner/drill DRILL-5752 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/940.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #940 commit ba3d1ed53d7cabc5f5ac860531f6e989f567bc65 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-30T19:53:49Z Removed redundant exclusion from java-exec pom Marked the slow tests in contrib as secondary tests commit 9a3b7d2e1de85ce4397311e9ff2433432982fb03 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-30T22:02:23Z Made slow unit tests SecondaryTests Changed the number of forked test processes from just 2 processes to 1.5 process per core commit 4159bf8f3ad4966751b9176843e240bb4885cec5 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T00:10:38Z Added JdbcTest category commit 0ab8ba8474b68bae8175f4b3c4a9b372f20eef8a Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T00:24:03Z Removed unused imports from tests commit bf3748c6ddd96b303762d8fde9046215cb243c02 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T00:34:22Z Added JdbcStorageTest Category commit 749fa9b1c4ae113a010de33bf359a38a1d56833e Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T18:15:18Z Created the HiveStorageTest category Made excludedGroups overrideable commit 50ddbb63f05eb08ac06d46dcf3c078e61dd3d042 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T18:39:28Z Added HbaseStorageTest category commit 10097f61cb2df8898c2e7d8ff20cfb63fddcb6c5 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T18:50:33Z Added the KuduStorageTest category commit 4dbad7edd5f479abc7db698e3ee2a2dad62abf20 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T19:06:34Z Added the MongoStorageTest Category commit 5077b7beeccddcb0c84c70645c879cd3168f1bc1 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T19:42:09Z Added a MemoryTest category commit a0e3832511041aeb18590d65ebb7166e40be045f Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T19:53:42Z Add another secondary test. commit efc9b8bb5af3ce891b890217ed1595d0db26cbbb Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:03:08Z Added a SecurityTest category Made more of the security tests secondary tests commit 8dfacb0715252e063eca7d7a51895b8da191b273 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:05:05Z Removed unused import for security test commit 72c3db7f1774fb0e37dd61a25ef1215f0391da63 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:05:57Z Added another memory test commit 3fd24f8062887253a27bbdb8f31f12a06aa97270 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:22:14Z Added an OperatorTest category commit 11290f9d48d223f8ca8b9c2df56e458616b51614 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:29:50Z Added license headers Added a ParquetTest category commit 3b03bb385466a2796578dd8ad96f2596a2b0f95e Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:33:35Z Added a VectorTest category commit 4d242150cb8228ba044ad96706f09524664773bb Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:55:00Z Added the SqlTest category commit 3cb3e895a9eb5171b88085c6984dbcb2685bbe58 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T20:59:14Z Added more Vector tests commit 5f6ef77b65966b430a3b77ec577d3f2627133b59 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T21:01:07Z Added more security tests commit 900911b2845e1a2b08d1f587afc505436ce6424e Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T21:03:26Z Added another sql test commit 0fc54d716a9920bfd768d28314f9cdc30bf8d0d5 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T21:04:50Z Added another security test commit 8e8c74b23646a3c6f203ae31468bf1e51787face Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T21:07:13Z Added another vector test commit 0b0f42a85d327b5ef723aa4ba58b30a5854c8ca6 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T21:14:33Z Added more operator and sql tests. commit f62ad15d3e0063032e55e857c898e08af7dc6084 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T21:39:01Z Added the PlannerTest category Categorized more tests commit 012e4a7b4048846e7ccca3b263c065daa1156d77 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T22:26:40Z Added the SqlFunctionsTest category Categorized more unit tests commit 04024ee2ae01d04070638e5e67f5dcc9cbc9305b Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-08-31T22:49:57Z Fixed checkstyle errors commit c2d63579aaeac88ede9e369dee67db49a9595fc4 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-09-01T04:21:21Z Moved test categories into their own package Fixed race condition with jdbc storage plugin tests Fixed prepared statement timeout with the drill-jdbc tests commit 47173b8616e5a08929f5a178206fbcb3431e1628 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-09-05T16:55:29Z Cleaned up jdbc storage plugin pom Increased the timeout of some unit tests Increased the prepared statement creation timeout for some unit tests commit e0c9a361c7e089bec5713defbd41228369336325 Author: Timothy Farkas <timothyfarkas@apache.org> Date: 2017-09-12T17:55:29Z Decrease to one test process per core
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on the issue:

          https://github.com/apache/drill/pull/940

          @paul-rogers This is an implementation of the Test Categories suggestion you made on the dev list. It's ready for review.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on the issue: https://github.com/apache/drill/pull/940 @paul-rogers This is an implementation of the Test Categories suggestion you made on the dev list. It's ready for review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138466695

          — Diff: common/src/test/java/org/apache/drill/categories/package-info.java —
          @@ -0,0 +1,23 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +/**
          + * This package stores the JUnit test categories.
          + */
          — End diff –

          Very cool. One thing that is not intuitively obvious here is the purpose and usage of the categories.

          On the one hand, we already have modules for memory, vectors, Hive, etc. One can easily run just those tests by cd'ing into that module and running Maven from there.

          So, one would imagine categories to provide another dimension, orthogonal to modules. For example, the classic "fast" vs. "slow." One could also imagine a core set of "smoke" tests that run quickly and do a quick validation. Then a set of standard tests that are more thorough. Then, very in-depth tests that either take a long time, or validate very specific bugs or regressions.

          Can you add a bit of commentary to advise how you envision the categories to be used?

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138466695 — Diff: common/src/test/java/org/apache/drill/categories/package-info.java — @@ -0,0 +1,23 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * This package stores the JUnit test categories. + */ — End diff – Very cool. One thing that is not intuitively obvious here is the purpose and usage of the categories. On the one hand, we already have modules for memory, vectors, Hive, etc. One can easily run just those tests by cd'ing into that module and running Maven from there. So, one would imagine categories to provide another dimension, orthogonal to modules. For example, the classic "fast" vs. "slow." One could also imagine a core set of "smoke" tests that run quickly and do a quick validation. Then a set of standard tests that are more thorough. Then, very in-depth tests that either take a long time, or validate very specific bugs or regressions. Can you add a bit of commentary to advise how you envision the categories to be used?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138475152

          — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java —
          @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException {
          // Connection-and other JDBC objects-on test method failure, but this test
          // class uses some objects across methods.)
          Driver.load();

          • connection = DriverManager.getConnection( "jdbc:drill:zk=local" );
            + Properties properties = new Properties();
            + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000");
              • End diff –

          Also, since properties don't have to be strings, why is this property stored as a string when the value is an int?

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138475152 — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java — @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException { // Connection- and other JDBC objects -on test method failure, but this test // class uses some objects across methods.) Driver.load(); connection = DriverManager.getConnection( "jdbc:drill:zk=local" ); + Properties properties = new Properties(); + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000"); End diff – Also, since properties don't have to be strings, why is this property stored as a string when the value is an int?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138472834

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java —
          @@ -54,11 +56,13 @@
          import org.junit.Ignore;
          import org.junit.Rule;
          import org.junit.Test;
          +import org.junit.experimental.categories.Category;
          import org.junit.rules.TemporaryFolder;
          import org.junit.runner.RunWith;
          import org.junit.runners.Parameterized;

          @RunWith(Parameterized.class)
          +@Category(

          {SecondaryTest.class, ParquetTest.class}

          )
          — End diff –

          On the other hand, the Parquet writer seems pretty important and we'd want to test it more often.

          This is why, in my earlier comment, I asked about the purpose of categorization: those that are worth running frequently ("smoke tests") vs. those that give a more thorough test, but will typically not find issues.

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138472834 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java — @@ -54,11 +56,13 @@ import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; +import org.junit.experimental.categories.Category; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @RunWith(Parameterized.class) +@Category( {SecondaryTest.class, ParquetTest.class} ) — End diff – On the other hand, the Parquet writer seems pretty important and we'd want to test it more often. This is why, in my earlier comment, I asked about the purpose of categorization: those that are worth running frequently ("smoke tests") vs. those that give a more thorough test, but will typically not find issues.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138475004

          — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java —
          @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException {
          // Connection-and other JDBC objects-on test method failure, but this test
          // class uses some objects across methods.)
          Driver.load();

          • connection = DriverManager.getConnection( "jdbc:drill:zk=local" );
            + Properties properties = new Properties();
            + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000");
              • End diff –

          I wonder, should we define a function for this pattern?

          ```
          ExecConstants.bootDefaultFor(
          ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS)
          ```

          But, we use Java 7, so the static function must reside elsewhere. Actually, the function might exist; I seem to recall making the same comment in the PR that externalized the defaults.

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138475004 — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java — @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException { // Connection- and other JDBC objects -on test method failure, but this test // class uses some objects across methods.) Driver.load(); connection = DriverManager.getConnection( "jdbc:drill:zk=local" ); + Properties properties = new Properties(); + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000"); End diff – I wonder, should we define a function for this pattern? ``` ExecConstants.bootDefaultFor( ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS) ``` But, we use Java 7, so the static function must reside elsewhere. Actually, the function might exist; I seem to recall making the same comment in the PR that externalized the defaults.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138472376

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestCorruptParquetDateCorrection.java —
          @@ -59,6 +61,7 @@

          • Use of this option is assumed to be extremely unlikely, but it is included
          • for completeness.
            */
            +@Category(ParquetTest.class)
              • End diff –

          Secondary? Would not seem that this test need to be run on each build...

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138472376 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestCorruptParquetDateCorrection.java — @@ -59,6 +61,7 @@ Use of this option is assumed to be extremely unlikely, but it is included for completeness. */ +@Category(ParquetTest.class) End diff – Secondary? Would not seem that this test need to be run on each build...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138473108

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java —
          @@ -148,10 +149,10 @@ public void sortOneKeyDescendingExternalSortLegacy() throws Throwable {

          private void sortOneKeyDescendingExternalSort(boolean testLegacy) throws Throwable {
          FixtureBuilder builder = ClusterFixture.builder()

          • .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4)
          • .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_GROUP_SIZE, 4)
          • .configProperty(ExecConstants.EXTERNAL_SORT_BATCH_LIMIT, 4)
          • .configProperty(ExecConstants.EXTERNAL_SORT_DISABLE_MANAGED, false)
            + .configProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4)
              • End diff –

          These options are boot options, not runtime options, so no need for the prefix (unless someone changed something.)

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138473108 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java — @@ -148,10 +149,10 @@ public void sortOneKeyDescendingExternalSortLegacy() throws Throwable { private void sortOneKeyDescendingExternalSort(boolean testLegacy) throws Throwable { FixtureBuilder builder = ClusterFixture.builder() .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4) .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_GROUP_SIZE, 4) .configProperty(ExecConstants.EXTERNAL_SORT_BATCH_LIMIT, 4) .configProperty(ExecConstants.EXTERNAL_SORT_DISABLE_MANAGED, false) + .configProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4) End diff – These options are boot options, not runtime options, so no need for the prefix (unless someone changed something.)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138489633

          — Diff: common/src/test/java/org/apache/drill/categories/package-info.java —
          @@ -0,0 +1,23 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +/**
          + * This package stores the JUnit test categories.
          + */
          — End diff –

          Currently there are two broad categories: *Fast* and *Secondary. The **Fast* tests are smoke tests. The *Secondary* tests are for slow tests. I can add a third *BugFix*category to represent tests for specific bugs.

          The rest of the categories represent broad groups of tests. Some of those categories have tests which span multiple class files and multiple packages. An example of such a category is *OperatorTest, **PlannerTest, or **SqlTest*. These categories are helpful when you are making a change in a particular area since they allow you to run all the tests for that area easily, and also effectively serve as a form of documentation. For example it would have been helpful to have an <b>OptionsTest</b> category when I was making the option system changes, since it took me a while to find all the relevant test classes and I had to construct a long maven command to run the tests.

          Also the structure of the tests as they are now is somewhat counter intuitive because tests can span different modules. For example consider the vector submodule. One would think all the relevant tests for vectors are in that submodule, but currently they are not. In fact there are no unit tests in the vector submodule and all relevant vector tests are in java-exec. Also the memory submodule has tests in it and a relevant test in the java-exec module as well.

          Some categories I agree are redundant like HiveStorageTest. Those tests have a clear one to one mapping to their submodule, however, I included a category for them as well for the sake of consistency. This way we can have a uniform mechanism for running subsets of tests instead of one mechanism for some groups of tests and another mechanism for other tests.

          To summarize, I can add a *BugFixTest* category to add another level of distinction between *Secondary* tests. But I am compelled to keep the other test categories intact for the reasons outlined above. Let me know what you think.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138489633 — Diff: common/src/test/java/org/apache/drill/categories/package-info.java — @@ -0,0 +1,23 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * This package stores the JUnit test categories. + */ — End diff – Currently there are two broad categories: * Fast * and * Secondary . The **Fast * tests are smoke tests. The * Secondary * tests are for slow tests. I can add a third * BugFix *category to represent tests for specific bugs. The rest of the categories represent broad groups of tests. Some of those categories have tests which span multiple class files and multiple packages. An example of such a category is * OperatorTest , **PlannerTest , or **SqlTest *. These categories are helpful when you are making a change in a particular area since they allow you to run all the tests for that area easily, and also effectively serve as a form of documentation. For example it would have been helpful to have an <b>OptionsTest</b> category when I was making the option system changes, since it took me a while to find all the relevant test classes and I had to construct a long maven command to run the tests. Also the structure of the tests as they are now is somewhat counter intuitive because tests can span different modules. For example consider the vector submodule. One would think all the relevant tests for vectors are in that submodule, but currently they are not. In fact there are no unit tests in the vector submodule and all relevant vector tests are in java-exec. Also the memory submodule has tests in it and a relevant test in the java-exec module as well. Some categories I agree are redundant like HiveStorageTest. Those tests have a clear one to one mapping to their submodule, however, I included a category for them as well for the sake of consistency. This way we can have a uniform mechanism for running subsets of tests instead of one mechanism for some groups of tests and another mechanism for other tests. To summarize, I can add a * BugFixTest * category to add another level of distinction between * Secondary * tests. But I am compelled to keep the other test categories intact for the reasons outlined above. Let me know what you think.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138497894

          — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java —
          @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException {
          // Connection-and other JDBC objects-on test method failure, but this test
          // class uses some objects across methods.)
          Driver.load();

          • connection = DriverManager.getConnection( "jdbc:drill:zk=local" );
            + Properties properties = new Properties();
            + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000");
              • End diff –

          I made ExecConstants a file class with a private constructor and added the static method there.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138497894 — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java — @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException { // Connection- and other JDBC objects -on test method failure, but this test // class uses some objects across methods.) Driver.load(); connection = DriverManager.getConnection( "jdbc:drill:zk=local" ); + Properties properties = new Properties(); + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000"); End diff – I made ExecConstants a file class with a private constructor and added the static method there.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138497914

          — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java —
          @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException {
          // Connection-and other JDBC objects-on test method failure, but this test
          // class uses some objects across methods.)
          Driver.load();

          • connection = DriverManager.getConnection( "jdbc:drill:zk=local" );
            + Properties properties = new Properties();
            + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000");
              • End diff –

          The Properties class only allows strings to be set as values. The conversion to an integer is done somewhere inside the DriverManager.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138497914 — Diff: exec/jdbc/src/test/java/org/apache/drill/jdbc/ConnectionTest.java — @@ -57,7 +61,9 @@ public static void setUpConnection() throws SQLException { // Connection- and other JDBC objects -on test method failure, but this test // class uses some objects across methods.) Driver.load(); connection = DriverManager.getConnection( "jdbc:drill:zk=local" ); + Properties properties = new Properties(); + properties.setProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS, "30000"); End diff – The Properties class only allows strings to be set as values. The conversion to an integer is done somewhere inside the DriverManager.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138499857

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java —
          @@ -54,11 +56,13 @@
          import org.junit.Ignore;
          import org.junit.Rule;
          import org.junit.Test;
          +import org.junit.experimental.categories.Category;
          import org.junit.rules.TemporaryFolder;
          import org.junit.runner.RunWith;
          import org.junit.runners.Parameterized;

          @RunWith(Parameterized.class)
          +@Category(

          {SecondaryTest.class, ParquetTest.class}

          )
          — End diff –

          To me the main distinction between smoke and secondary tests should be runtime. It's hard in my mind to distinguish based on importance, because the relevance of a test depends on the change being done. And you would end up running all the tests before finishing your change no matter what. The workflow I had in mind was the following:

          1. Make a change
          1. Run Fast tests for my category of interest (e.g. *OperatorTest*). Oops there's a failure
          1. Fix the failure
          1. Run Fast tests for category *OperatorTest* again. Oops another failure
          1. Fix the failure
          1. Run Fast tests for category *OperatorTest* again. Yay they pass
          1. Run all the tests
          1. Done

          Let me know what you think.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138499857 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java — @@ -54,11 +56,13 @@ import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; +import org.junit.experimental.categories.Category; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @RunWith(Parameterized.class) +@Category( {SecondaryTest.class, ParquetTest.class} ) — End diff – To me the main distinction between smoke and secondary tests should be runtime. It's hard in my mind to distinguish based on importance, because the relevance of a test depends on the change being done. And you would end up running all the tests before finishing your change no matter what. The workflow I had in mind was the following: 1. Make a change 1. Run Fast tests for my category of interest (e.g. * OperatorTest *). Oops there's a failure 1. Fix the failure 1. Run Fast tests for category * OperatorTest * again. Oops another failure 1. Fix the failure 1. Run Fast tests for category * OperatorTest * again. Yay they pass 1. Run all the tests 1. Done Let me know what you think.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138505403

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java —
          @@ -148,10 +149,10 @@ public void sortOneKeyDescendingExternalSortLegacy() throws Throwable {

          private void sortOneKeyDescendingExternalSort(boolean testLegacy) throws Throwable {
          FixtureBuilder builder = ClusterFixture.builder()

          • .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4)
          • .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_GROUP_SIZE, 4)
          • .configProperty(ExecConstants.EXTERNAL_SORT_BATCH_LIMIT, 4)
          • .configProperty(ExecConstants.EXTERNAL_SORT_DISABLE_MANAGED, false)
            + .configProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4)
              • End diff –

          Done, thanks for catching.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138505403 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/xsort/TestSimpleExternalSort.java — @@ -148,10 +149,10 @@ public void sortOneKeyDescendingExternalSortLegacy() throws Throwable { private void sortOneKeyDescendingExternalSort(boolean testLegacy) throws Throwable { FixtureBuilder builder = ClusterFixture.builder() .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4) .configProperty(ExecConstants.EXTERNAL_SORT_SPILL_GROUP_SIZE, 4) .configProperty(ExecConstants.EXTERNAL_SORT_BATCH_LIMIT, 4) .configProperty(ExecConstants.EXTERNAL_SORT_DISABLE_MANAGED, false) + .configProperty(OptionValidator.OPTION_DEFAULTS_ROOT + ExecConstants.EXTERNAL_SORT_SPILL_THRESHOLD, 4) End diff – Done, thanks for catching.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138506701

          — Diff: common/src/test/java/org/apache/drill/categories/package-info.java —
          @@ -0,0 +1,23 @@
          +/**
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing, software
          + * distributed under the License is distributed on an "AS IS" BASIS,
          + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + * See the License for the specific language governing permissions and
          + * limitations under the License.
          + */
          +
          +/**
          + * This package stores the JUnit test categories.
          + */
          — End diff –

          Terminology update. I'm renaming *SecondaryTest* to *SlowTest* and instead of *BugFixTest* I will create a category called *UnlikelyTest. **UnlikelyTest* will be a category for tests that excercise specific bug fixes, or are for tests that you're unlikely to break.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138506701 — Diff: common/src/test/java/org/apache/drill/categories/package-info.java — @@ -0,0 +1,23 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * This package stores the JUnit test categories. + */ — End diff – Terminology update. I'm renaming * SecondaryTest * to * SlowTest * and instead of * BugFixTest * I will create a category called * UnlikelyTest . **UnlikelyTest * will be a category for tests that excercise specific bug fixes, or are for tests that you're unlikely to break.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138506769

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java —
          @@ -54,11 +56,13 @@
          import org.junit.Ignore;
          import org.junit.Rule;
          import org.junit.Test;
          +import org.junit.experimental.categories.Category;
          import org.junit.rules.TemporaryFolder;
          import org.junit.runner.RunWith;
          import org.junit.runners.Parameterized;

          @RunWith(Parameterized.class)
          +@Category(

          {SecondaryTest.class, ParquetTest.class}

          )
          — End diff –

          Please see Terminology update in comment above.

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138506769 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestParquetWriter.java — @@ -54,11 +56,13 @@ import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; +import org.junit.experimental.categories.Category; import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @RunWith(Parameterized.class) +@Category( {SecondaryTest.class, ParquetTest.class} ) — End diff – Please see Terminology update in comment above.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on a diff in the pull request:

          https://github.com/apache/drill/pull/940#discussion_r138683227

          — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestCorruptParquetDateCorrection.java —
          @@ -59,6 +61,7 @@

          • Use of this option is assumed to be extremely unlikely, but it is included
          • for completeness.
            */
            +@Category(ParquetTest.class)
              • End diff –

          Done

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on a diff in the pull request: https://github.com/apache/drill/pull/940#discussion_r138683227 — Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/writer/TestCorruptParquetDateCorrection.java — @@ -59,6 +61,7 @@ Use of this option is assumed to be extremely unlikely, but it is included for completeness. */ +@Category(ParquetTest.class) End diff – Done
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user paul-rogers commented on the issue:

          https://github.com/apache/drill/pull/940

          Thanks for the explanations and corrections.
          Please squash your commits to prepare for commit to master.
          +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user paul-rogers commented on the issue: https://github.com/apache/drill/pull/940 Thanks for the explanations and corrections. Please squash your commits to prepare for commit to master. +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on the issue:

          https://github.com/apache/drill/pull/940

          Thanks @paul-rogers squashed and ready to commit

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on the issue: https://github.com/apache/drill/pull/940 Thanks @paul-rogers squashed and ready to commit
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on the issue:

          https://github.com/apache/drill/pull/940

          Please merge my other PR https://github.com/apache/drill/pull/923 before this one. Thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on the issue: https://github.com/apache/drill/pull/940 Please merge my other PR https://github.com/apache/drill/pull/923 before this one. Thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ilooner commented on the issue:

          https://github.com/apache/drill/pull/940

          Don't merge this. I need to test this on jenkins with the latest fixes in https://github.com/apache/drill/pull/945

          Show
          githubbot ASF GitHub Bot added a comment - Github user ilooner commented on the issue: https://github.com/apache/drill/pull/940 Don't merge this. I need to test this on jenkins with the latest fixes in https://github.com/apache/drill/pull/945

            People

            • Assignee:
              timothyfarkas Timothy Farkas
              Reporter:
              timothyfarkas Timothy Farkas
              Reviewer:
              Paul Rogers
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:

                Development