impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vihang Karajgaonkar (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-8632: Add support for self-event detection for insert events
Date Tue, 07 Apr 2020 22:21:40 GMT
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

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message