Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12601 )
Change subject: IMPALA-8240: Event processor should keep trying when metastore is unavailable. ...................................................................... Patch Set 4: (9 comments) Bunch of nits. http://gerrit.cloudera.org:8080/#/c/12601/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12601/4//COMMIT_MSG@12 PS4, Line 12: thise typo http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java File fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java: http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@25 PS4, Line 25: nit: extra \n http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@27 PS4, Line 27: * A MetastoreEventsProcessor test class useful for testing impacts of : * HMS crashes/restarts on MetastoreEventsProcessor status. : */ I'd rephrase this to something like, events processor that simulates HMS failures... http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@30 PS4, Line 30: ForTests remove. http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@31 PS4, Line 31: extends MetastoreEventsProcessor { indentation off http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@36 PS4, Line 36: private int counter = 0; class members use _ suffix. Move to the top. Add a comment http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@42 PS4, Line 42: counter < 3 This looks weird. How about throw an error randomly 50% of the times? if (rand.nextInt() % 2) throw http://gerrit.cloudera.org:8080/#/c/12601/4/fe/src/test/java/org/apache/impala/catalog/events/HMSFetchNotificationsEventProcessorForTests.java@45 PS4, Line 45: else { not needed. http://gerrit.cloudera.org:8080/#/c/12601/4/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/12601/4/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@529 PS4, Line 529: fetchProcessor_ local variables don't need a suffix _ -- To view, visit http://gerrit.cloudera.org:8080/12601 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I570a52462e6d3b634b2c227dfcb98e20ad2a0023 Gerrit-Change-Number: 12601 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada <anuragmantr...@gmail.com> Gerrit-Reviewer: Anurag Mantripragada <anuragmantr...@gmail.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Wed, 27 Feb 2019 03:20:18 +0000 Gerrit-HasComments: Yes
