Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15648 )
Change subject: IMPALA-8632: Add support for self-event detection for insert events ...................................................................... Patch Set 4: (9 comments) mostly looks good to me. Should be okay once we add the suggestions. http://gerrit.cloudera.org:8080/#/c/15648/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/15648/3/be/src/common/global-flags.cc@254 PS3, Line 254: : DEFINE_int32(hms_event_polling_interval_s, 0, > I think this is implementation detail and can be skipped. Also the summary Are you planning to update this in a subsequent patch? http://gerrit.cloudera.org:8080/#/c/15648/4/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/15648/4/bin/impala-config.sh@177 PS4, Line 177: export CDP_BUILD_NUMBER=2506706 The changes in this file should be made in a separate change. Also, this build number has a problem which causes a test failure. I uploaded a different patch to bump it up a better build. https://gerrit.cloudera.org/#/c/15668/. May be revert the changes to this file and rebase on top of my patch linked above? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java@555 PS4, Line 555: null May be change this to Collections.emptyList() so that the callers don't need to check for null. http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@997 PS4, Line 997: public static class InsertEventInfo { this duplicates a lot of code between the two version of the MetastoreShim. I think we can keep the existing fireInsertEvent method in the MetastoreUtils method. Here we can just make use of the return types. For instance, in hive-2 shim we do public static List<Long> fireInsertEvent(IMetaStoreClient msClient, List<InsertEventInfo> insertEventInfos) throws TException { MetastoreUtil.fireInsertEvent(msClient, insertEventInfos); return Collections.emptyList(); } in hive-3 shim we do public static List<Long> fireInsertEvent(IMetaStoreClient msClient, List<InsertEventInfo> insertEventInfos) throws TException { FireEventResponse resp = MetastoreUtil.fireInsertEvent(msClient, insertEventInfos); Preconditions.checkState(resp.getEventIds() != null && !resp.getEventIds().isEmpty()); return resp.getEventIds(); } http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/SelfEventContext.java File fe/src/main/java/org/apache/impala/catalog/events/SelfEventContext.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/SelfEventContext.java@35 PS4, Line 35: private final long idFromEvent_; rename to insertEventId_? http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/catalog/events/SelfEventContext.java@69 PS4, Line 69: idFromEvent_ = eventId; can you add a preconditions check that this eventId>0 http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4487 PS4, Line 4487: // Firing insert events by making calls to HMS APIs can be slow for tables with : // large number of partitions. this comment should be updated with the bulk API it should not be a problem anymore. May be add a TODO to evaluate the performance of bulk API http://gerrit.cloudera.org:8080/#/c/15648/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@4506 PS4, Line 4506: e can be moved to earlier line. http://gerrit.cloudera.org:8080/#/c/15648/3/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/15648/3/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@725 PS3, Line 725: Assume.assumeTrue("Skipping this test because it only works with Hive-3 or greater", > Can you also enable the which is commented out here: https://github.com/apa ping -- To view, visit http://gerrit.cloudera.org:8080/15648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7873fbb2c159343690f93b9d120f6b425b983dcf Gerrit-Change-Number: 15648 Gerrit-PatchSet: 4 Gerrit-Owner: Xiaomeng Zhang <[email protected]> Gerrit-Reviewer: Anurag Mantripragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Xiaomeng Zhang <[email protected]> Gerrit-Comment-Date: Tue, 07 Apr 2020 22:21:40 +0000 Gerrit-HasComments: Yes
