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 <xiaomeng@cloudera.com>
Gerrit-Reviewer: Anurag Mantripragada <anurag@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vihang@cloudera.com>
Gerrit-Reviewer: Xiaomeng Zhang <xiaomeng@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Apr 2020 22:21:40 +0000
Gerrit-HasComments: Yes
|