Sai Hemanth Gantasala has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/19378 )

Change subject: IMPALA:11808: Added support for reload event in catalogD
......................................................................


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/19378/6/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

http://gerrit.cloudera.org:8080/#/c/19378/6/be/src/catalog/catalog-server.cc@127
PS6, Line 127: This configuration is used "
             :     "to fire a refresh/invalidate table event to the HMS and 
other event processors "
             :     "can process this event. The default value is false, so 
impala will not fire this "
             :     "event. If enabled, impala will fire this event and other 
catalogD will process it
> It is not clear to me from the description that this config affects both fi
The config only affects the firing of the event. Processing will not be 
affected by this config. Added more details to this config.


http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java
File 
fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java:

http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@442
PS6, Line 442:     return Collections.EMPTY_LIST;
> I would prefer throwing UnsupportedOperationException
Ack


http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java@447
PS6, Line 447: MetastoreNotificationException
> UnsupportedOperationException seem more fitting here.
Ack


http://gerrit.cloudera.org:8080/#/c/19378/6/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/19378/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2474
PS6, Line 2474: enableProcessingReloadEvent
> Isn't it called enableReloadEvents()?
Will remove this if clause as we don't want to restrict the processing of 
reload event.


http://gerrit.cloudera.org:8080/#/c/19378/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2476
PS6, Line 2476: will
> will not be processed?
same as above comment



--
To view, visit http://gerrit.cloudera.org:8080/19378
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic62d58837d356dc2113f3c0904228ac9de484136
Gerrit-Change-Number: 19378
Gerrit-PatchSet: 6
Gerrit-Owner: Sai Hemanth Gantasala <[email protected]>
Gerrit-Reviewer: Abhishek Rawat <[email protected]>
Gerrit-Reviewer: Csaba Ringhofer <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]>
Gerrit-Comment-Date: Wed, 04 Jan 2023 21:58:03 +0000
Gerrit-HasComments: Yes

Reply via email to