Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/13932 )
Change subject: IMPALA-8661 : Add randomized tests to stress MetastoreEventsProcessor ...................................................................... Patch Set 5: (14 comments) Great work, exhaustive coverage. I skimmed through most parts of the patch. seems reasonable to me. I think it is difficult to review the entire patch in a single go. We should probably commit it and keep fixing any issues incrementally. Since most of the patch is isolated to test framework, any bugs (like connection leaks, if any) shouldn't affect the product itself. http://gerrit.cloudera.org:8080/#/c/13932/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13932/5//COMMIT_MSG@35 PS5, Line 35: 1. Ran the test with defaults. It generates about 2100 events Should we consider lower defaults for core tests? 15mins seems like a considerable increase in test execution time. http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@299 PS5, Line 299: infoLog("Notification event received"); nit: isn't this too chatty for info? http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/catalog/events/EventsProcessorStressTest.java File fe/src/test/java/org/apache/impala/catalog/events/EventsProcessorStressTest.java: http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/catalog/events/EventsProcessorStressTest.java@46 PS5, Line 46: refresh nit: grammar http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/catalog/events/EventsProcessorStressTest.java@83 PS5, Line 83: else do we want to override numClients in 3.x? in which case it should probably be something like if (version < 3 && numClients != null)... http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@138 PS5, Line 138: import org.mockito.Mockito; remove? http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java File fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java: http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java@28 PS5, Line 28: import java.util.concurrent.BlockingQueue; nit: duplicate. http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java@36 PS5, Line 36: public class HiveJdbcClientPool implements Closeable { class comment. http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java@71 PS5, Line 71: if (conn_ == null) { : throw new RuntimeException("Connection not initialized."); : } else if (conn_.isClosed()) { : throw new RuntimeException("Connection not open."); : } Make them preconditions? http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java@94 PS5, Line 94: public boolean executeSql(String sql) throws SQLException { method doc. http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/HiveJdbcClientPool.java@146 PS5, Line 146: closedCount > 0 !freeClients_.isEmpty() ? http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/TestUtils.java File fe/src/test/java/org/apache/impala/testutil/TestUtils.java: http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@42 PS5, Line 42: import org.apache.commons.lang3.RandomStringUtils; remove? http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@77 PS5, Line 77: DEFAULT_CONNECTION_TEMPLATE nit:HS2_CONNECTION_TEMPLATE? http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/util/RandomHiveQueryRunner.java File fe/src/test/java/org/apache/impala/util/RandomHiveQueryRunner.java: http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/util/RandomHiveQueryRunner.java@117 PS5, Line 117: for nit: remove http://gerrit.cloudera.org:8080/#/c/13932/5/fe/src/test/java/org/apache/impala/util/RandomHiveQueryRunner.java@345 PS5, Line 345: exists exist -- To view, visit http://gerrit.cloudera.org:8080/13932 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c85b83efd4f56b5ae0e8d1dc6a2ee2feb6721ce Gerrit-Change-Number: 13932 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 30 Jul 2019 07:32:53 +0000 Gerrit-HasComments: Yes
