You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org> on 2019/02/05 05:54:00 UTC

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Vihang Karajgaonkar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12365

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

This change adds the ability to disable event processing at a table or
database level. If the user adds a property with the key
"impala.disableEventMdSync" in the table or database properties, event
processing can be turned on/off at the respective level. The value of
the property is string value of Booleans ("true" or "false"). By default
event processing is enabled when the global config
hms_event_polling_interval_s is set to a non-zero value. The value of
property determines if the event processing needs to be disabled for a
particular table or database. So for example, if
"impala.disableEventMdSync" is set to "true" then events for that
table/database are ignored. If the property is unset or a value of
"false" is used, the event processing is enabled (although it is not
necessary to explicitly enable event processing, given the global flag).

When both table and db level properties are set, the table level
property takes precedence. If the table level property is not set, then
database level property is used to evaluate if the event needs to be
processed or not.

Users can issue alter table command to change the value of this
property. When the flag value is changed, the alter table event
generated is always processed and subsequent events are either ignored
or processed based on the new value of table flag and database flag. In
theory, a database level property can also be changed. But Impala
doesn't currently support alter database operations. Users who may wish
to change the database level property should use alter database command
from Apache Hive (or any other metastore client)

Testing done: Added new unit tests which execute all supported DDL
operations with all possible combination of db and table flags. Another
test executes alter table operations for changing flags for all combinations
of transitions at table level.

Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
4 files changed, 688 insertions(+), 64 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/1
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 1:

(10 comments)

Thanks for this addition. Great safety-valve for rapidly-changing tables that Impala does not need to track.

http://gerrit.cloudera.org:8080/#/c/12365/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12365/1//COMMIT_MSG@11
PS1, Line 11: "impala.disableEventMdSync" in the table or database properties, event
Suggestion: impala.disableHmsSync

Reason: "Md" is redundant since HMS is all about metadata. "Event" is redundant because events are an implementation detail. The only other "Sync" we might have is HDFS sync. Adding "Hms" allows us to add a "disableHdfsSync" option later.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@831
PS1, Line 831:    * parameters
Maybe note that the property is fetched only from the cached table; returns null if the table is not yet loaded.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@846
PS1, Line 846: 
Nit: remove blank line


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@393
PS1, Line 393:       if (isEventProcessingDisabled()) return;
The table does not yet exist, so checking the cached table to check if it wants to be refreshed seems ineffective.

Suppose we add the table. We must refresh the table to find out if we should have refreshed the table. This loads unwanted tables.

Suppose the table expires out of the cache (because it is never used.) The next change to the table will cause us to refresh the table (so we can, again, find out we should not have honored the event.)

So, question: should we mark even the stub table with whether we want sync? Can we obtain that value from the information given in the create table message (assuming the user set the property on the CREATE TABLE statement)?


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@400
PS1, Line 400:         tableAdded = catalog_.addTableIfNotExists(dbName_, tblName_);
Move variable declaration inside try. Turns out no other code references it.

In fact, since it is not used, it is not needed at all.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@492
PS1, Line 492:       String afterFlag = tableAfter_.getParameters().get(DISABLE_EVENT_MDSYNC_KEY);
Do you need to check to see if parameters are null here? Or, are we assured that the parameters object itself always exists?


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@493
PS1, Line 493:       eventSyncFlagChanged_ = !StringUtils.endsWithIgnoreCase(beforeFlag, afterFlag);
This would be somewhat easier to follow if both parameters were converted to booleans and to booleans compared.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@768
PS1, Line 768:       if (isEventProcessingDisabled()) return;
This block has appeared many times now. Would it make sense to do the following:

base class:
void process() {
  if (!is...Disabled()) doProcess();
}

With all subclass table methods implementing doProcess instead? (Feel free to pick a name less goofy than "doProcess"...)


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@818
PS1, Line 818:       }
Refactor into a base class method (perhaps static) rather than copy/paste the block multiple times?

In fact, with a bit of Java magic, you can factor out the above block also.

<T extends NotificationEvent> T getEvent(NotificationEent event, Class<T> eventClass) {
  Preconditions.checkArgument(eventClass.isInstance(event))
  ...
  return eventClass.cast(event);


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@840
PS1, Line 840:       return isEventProcessingDisabled_;
Can this be on the base class? Overridden only when different?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Feb 2019 23:39:53 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 12: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 21:14:07 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1984/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 06:39:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12365/2/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/12365/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@858
PS2, Line 858:       tblParams.put(MetastoreEventUtils.DISABLE_EVENT_MDSYNC_KEY, String.valueOf(tblFlag));
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1075
PS2, Line 1075:   private void createDatabase(String dbName, Map<String, String> params) throws TException {
> line too long (92 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Feb 2019 00:16:20 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.

Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 8: Code-Review+1

I didn't get a chance to look at the tests again. The general logic/flow looks good to me. Will let Paul take a deeper pass and +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Feb 2019 19:59:02 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

This change adds the ability to disable event processing at a table or
database level. If the user adds a property with the key
"impala.disableHmsSync" in the table or database properties, event
processing can be turned on/off at the respective level. The value of
the property is string value of Booleans ("true" or "false"). By default
event processing is enabled when the global config
hms_event_polling_interval_s is set to a non-zero value. The value of
property determines if the event processing needs to be disabled for a
particular table or database. So for example, if
"impala.disableHmsSync" is set to "true" then events for that
table/database are ignored. If the property is unset or a value of
"false" is used, the event processing is enabled (although it is not
necessary to explicitly enable event processing, given the global flag).

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/2
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12365/8/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/12365/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@815
PS8, Line 815:         .thenReturn(dbFlag);
> Here I wonder I we can simply define a test-only catalog service subclass t
Is the concern related to using mockito in general or just to mock our own code. I think I can do away with mocking CatalogServiceCatalog here. But I may need it to mock NotificationEvent (Hive code). Will that approach be more reasonable?

The first version of this test didn't use any a mocks. It was suggested in the reviews that perhaps using a fake event could make it easier. When I start going that path, I found myself writing lot of fake dependent classes which I didn't really cared as far as this test was concerned. Mockito helps a lot in such cases, since you can exercise exactly the few lines of code which you want to test without bringing in the whole lot of dependent and unrelated code.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Feb 2019 22:53:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12365/5/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/12365/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@100
PS5, Line 100:     private static final Logger LOG = LoggerFactory.getLogger(MetastoreEventFactory.class);
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/12365/5/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/12365/5/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@805
PS5, Line 805:     when(mockNotificationEvent.getEventId()).thenReturn(eventIdGenerator.incrementAndGet());
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/12365/5/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1128
PS5, Line 1128:   private org.apache.hadoop.hive.metastore.api.Table getTestTable(String dbName, String tblName,
line too long (96 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:01:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

When the flag is turned OFF and the previous value of ON or unset, it is
possible that the table does not exist in catalog anymore. For example,
when the table was created the event sync was disabled. Hence the create
event was skipped. Now when the user re-enables the sync again on such
table, event processor has no idea how many events were skipped during
this interval. Event processor errs on the side of caution and changes
the state to NEEDS_INVALIDATE in such case.

Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
D fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreNotificationNeedsInvalidateException.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
6 files changed, 1,651 insertions(+), 752 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/10
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/4
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 4
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
---
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
R fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M impala-parent/pom.xml
6 files changed, 856 insertions(+), 131 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/7
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12365/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/12365/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@100
PS6, Line 100:     private static final Logger LOG = LoggerFactory.getLogger(MetastoreEventFactory.class);
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/12365/6/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/12365/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@804
PS6, Line 804:     when(mockNotificationEvent.getEventId()).thenReturn(eventIdGenerator.incrementAndGet());
line too long (92 > 90)


http://gerrit.cloudera.org:8080/#/c/12365/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1127
PS6, Line 1127:   private org.apache.hadoop.hive.metastore.api.Table getTestTable(String dbName, String tblName,
line too long (96 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:03:23 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3785/ DRY_RUN=false


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 21:14:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2131/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:17:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2089/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 20:58:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 11:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2132/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:21:04 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@850
PS2, Line 850:   }
> nit: newline
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@52
PS2, Line 52: DISABLE_EVENT_MDSYNC_KEY
> nit: DISABLE....*HMS*_SYNC.
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@165
PS2, Line 165:       if (events.isEmpty()) { return Collections.emptyList(); }
> nit: We don't use braces for onliner ifs.
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@276
PS2, Line 276: this table or database
> I'd rephrase it to.. disabled for the table/Db associated with this event t
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@320
PS2, Line 320:     @Override
> Method doc for this, I think the precedence order, tbl first/db next etc...
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@371
PS2, Line 371: ge is " + "
> merge?
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@379
PS2, Line 379:  " + "eve
> fix? Include the exception trace?
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@490
PS2, Line 490: msTbl_.isSetParameters() ? msTbl_.getParameters().get
> can probably be factored out into a helper function. Used thrice in this pa
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@492
PS2, Line 492: msTbl_
> tableAfter_?
Done


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@506
PS2, Line 506:       if (isEventProcessingDisabled()) return;
> Do we need to keep track of how many events we skipped/how many processed/h
Thanks for the suggestion. Made a note of this for the metrics patch.


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@364
PS2, Line 364: } finally {
             :       return lastProcessedEvent;
             :     }
> Does this swallow any exceptions from propagating to the caller?
hmm .. I was not aware of this. Thanks for pointing this out. Refactored the code to avoid returning from finally


http://gerrit.cloudera.org:8080/#/c/12365/2/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/12365/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@744
PS2, Line 744:   private void runDDLTestForFlagTransition(String dbName, String tblName,
> I think you could simplify the test like follows. Thoughts?
The test does much more than what is suggested above. It also needs to take into account the dbFlag value and make sure that the subsequent events are disabled/enabled based on the transition. If you are suggesting the using mocks would simplify the tests, then yes, I agree. In order to do this using mocks we will have to do the following:

1. I noticed that fe pom.xml does not have any testing framework using mocks. Mockito would be a great addition to the unit test framework in fe. Added a test dependency for mockito (latest released version)

2. Using mockito, now you can generate a mock notification and create a mock catalogServiceCatalog. We can use these mocks to initialize a AlterTableEvent and make sure that isEventProcessingDisabled() for this event is returned expected value based on tblFlag and dbFlag.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 00:26:56 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12365/2/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/12365/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@858
PS2, Line 858:       tblParams.put(MetastoreEventUtils.DISABLE_EVENT_MDSYNC_KEY, String.valueOf(tblFlag));
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1075
PS2, Line 1075:   private void createDatabase(String dbName, Map<String, String> params) throws TException {
line too long (92 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Feb 2019 01:04:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12365/8/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/12365/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@815
PS8, Line 815:         .thenReturn(dbFlag);
> Is the concern related to using mockito in general or just to mock our own 
No problem. This was more a general observation that using purpose-built classes is great when we can do it, but if Mockito is the right tool for the job, by all means let's use it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Feb 2019 21:29:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12365/1/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/12365/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@858
PS1, Line 858:       tblParams.put(MetastoreEventUtils.DISABLE_EVENT_MDSYNC_KEY, String.valueOf(tblFlag));
line too long (91 > 90)


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1075
PS1, Line 1075:   private void createDatabase(String dbName, Map<String, String> params) throws TException {
line too long (92 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 Feb 2019 05:54:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/11
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.

Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@850
PS2, Line 850:   }
nit: newline


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@52
PS2, Line 52: DISABLE_EVENT_MDSYNC_KEY
nit: DISABLE....*HMS*_SYNC.


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@165
PS2, Line 165:       if (events.isEmpty()) { return Collections.emptyList(); }
nit: We don't use braces for onliner ifs.


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@276
PS2, Line 276: this table or database
I'd rephrase it to.. disabled for the table/Db associated with this event to be more precise.


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@320
PS2, Line 320:     @Override
Method doc for this, I think the precedence order, tbl first/db next etc...?


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@371
PS2, Line 371: ge is " + "
merge?


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@379
PS2, Line 379:  " + "eve
fix? Include the exception trace?


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@490
PS2, Line 490: msTbl_.isSetParameters() ? msTbl_.getParameters().get
can probably be factored out into a helper function. Used thrice in this patch AFAICT.


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@492
PS2, Line 492: msTbl_
tableAfter_?


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@506
PS2, Line 506:       if (isEventProcessingDisabled()) return;
Do we need to keep track of how many events we skipped/how many processed/how many failed in a global state and periodically dump it? Can probably be done as a separate change (along with the metrics)


http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java:

http://gerrit.cloudera.org:8080/#/c/12365/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@364
PS2, Line 364: } finally {
             :       return lastProcessedEvent;
             :     }
Does this swallow any exceptions from propagating to the caller?
https://stackoverflow.com/questions/18205493/can-we-use-return-in-finally-block


http://gerrit.cloudera.org:8080/#/c/12365/2/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/12365/2/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@744
PS2, Line 744:   private void runDDLTestForFlagTransition(String dbName, String tblName,
I think you could simplify the test like follows. Thoughts?

 class FakeTableEventHanlder extends TableEventHandler {
   eventProcessed = false;
  @Override
  process() {
     if(eventProcessingDisabled()) return;
     eventProcessed = true;
  }
}

allEventTypes = {key:null, key:true, key:false}

for (event e1: allTypesEvents) {
  for (event e2: allTypesEvents) {
     shouldIncrement = hasStateChangedBetween(e1, e2)
     FakeTableEventHandler f;
     f.processEvent(e1)
     f.processEvent(e2)
     assert shouldIncrement ^ f.eventProcessed();
  }
}



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Feb 2019 00:59:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
D fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreNotificationNeedsInvalidateException.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M impala-parent/pom.xml
7 files changed, 1,652 insertions(+), 752 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/9
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 12: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 12
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Sat, 16 Feb 2019 01:08:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12365/9/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/12365/9/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@879
PS9, Line 879:     ((FakeCatalogServiceCatalogForFlagTests) fakeCatalog).setFlags(dbName, tblName, dbFlag,
line too long (91 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Feb 2019 23:34:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@768
PS1, Line 768:       if (isEventProcessingDisabled()) return;
> This block has appeared many times now. Would it make sense to do the follo
Good suggestion. Done. I introduced a new method which is called processIfEnabled() in the base class and it calls process() and is..Disabled()


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@818
PS1, Line 818:       }
> Sorry, referring to the fact that this same block of code appears multiple 
Thanks for clarifying. Unfortunately, I cannot do this in the base class

protected <T extends EventMessage> getTableObj (Class<T> messageClass, T message) {
 try {
   return message.getTableObj();
 } catch {
   ...
 }
}

because EventMessage does not define getTableObj().

I can use reflection to invoke then method by providing the name but I think it unnecessarily convolutes the stack traces in the logs and I find it hard to debug (you cannot search all accessors of the such method invocations using IDE for instance).

Hope this makes it a clearer. If not, can you please provide a example in this context in case I am missing something.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Feb 2019 00:06:00 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 7:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12365/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/12365/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@100
PS6, Line 100: 
> line too long (91 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/12365/6/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/12365/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@804
PS6, Line 804:     when(mockNotificationEvent.getEventId())
> line too long (92 > 90)
Done


http://gerrit.cloudera.org:8080/#/c/12365/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1127
PS6, Line 1127: 
> line too long (96 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:05:22 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/5
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12365/10/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/12365/10/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@879
PS10, Line 879:     ((FakeCatalogServiceCatalogForFlagTests) fakeCatalog).setFlags(dbName, tblName, dbFlag,
line too long (91 > 90)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 10
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Feb 2019 23:38:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 2:

(2 comments)

Looks like the style nagger has a couple of complaints. Also, please rebase. Looks like just one open suggestion, then we're good to go.

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@818
PS1, Line 818:         throw new MetastoreNotificationException(ex);
> Its unclear which block of code this comment is referring to. Can you pleas
Sorry, referring to the fact that this same block of code appears multiple times, with just the class name differing. Suggesting how to create a function that takes the class as a parameter to consolidate similar code blocks.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@840
PS1, Line 840:     public boolean isEventProcessingDisabled() {
> DropPartitionEvent is a special case since it doesn't have the table object
Ack



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Feb 2019 01:50:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/6
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2080/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:42:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2081/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 6
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:45:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2079/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 4
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:37:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 1:

(9 comments)

First set of replies, working on rest of comments which haven't been responded.

http://gerrit.cloudera.org:8080/#/c/12365/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/12365/1//COMMIT_MSG@11
PS1, Line 11: "impala.disableEventMdSync" in the table or database properties, event
> Suggestion: impala.disableHmsSync
Good suggestion. Changed the flag to impala.disableHmsSync


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@831
PS1, Line 831:    * parameters
> Maybe note that the property is fetched only from the cached table; returns
Done


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@846
PS1, Line 846: 
> Nit: remove blank line
Done


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java:

http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@393
PS1, Line 393:       if (isEventProcessingDisabled()) return;
> The table does not yet exist, so checking the cached table to check if it w
It is actually fetched from the event message's table object. The msTbl_ field is initialized by parsing the event message. The property value is checked from this msTbl_ object. In case of database level property though, we don't get the database object in each event. Hence in that case, we have to lookup the value of the property from the cached database. But then, in catalog, all the databases are generally always present unless user creates a brand new database which catalog is not aware of at that instant. In such case, the create_database event uses the database object from event instead of cached value of database.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@400
PS1, Line 400:         tableAdded = catalog_.addTableIfNotExists(dbName_, tblName_);
> Move variable declaration inside try. Turns out no other code references it
Done


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@492
PS1, Line 492:       String afterFlag = tableAfter_.getParameters().get(DISABLE_EVENT_MDSYNC_KEY);
> Do you need to check to see if parameters are null here? Or, are we assured
In general, they should always be set since these are heavily used all over the place by many projects. But it is not guaranteed, so I might as well put in a check here.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@493
PS1, Line 493:       eventSyncFlagChanged_ = !StringUtils.endsWithIgnoreCase(beforeFlag, afterFlag);
> This would be somewhat easier to follow if both parameters were converted t
My first implementation was using the suggested approach until I realized that if the beforeFlag is null and afterFlag is false, it is a valid flag modification case. The first case means: look at dbFlag and the second case means, tblFlag is explicitly set to false (ignore dbFlag).

If we use Boolean.valueOf(string_value) both the before and after flags will be False since null maps to a False. I had added a comment in the hopes that its is clear to the reader why we are not using Booleans here.


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@818
PS1, Line 818:       }
> Refactor into a base class method (perhaps static) rather than copy/paste t
Its unclear which block of code this comment is referring to. Can you please clarify?


http://gerrit.cloudera.org:8080/#/c/12365/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java@840
PS1, Line 840:       return isEventProcessingDisabled_;
> Can this be on the base class? Overridden only when different?
DropPartitionEvent is a special case since it doesn't have the table object (yet, there is a patch which Bharath merged only yesterday). Hence this the overridden method rather than the common case.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 1
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Feb 2019 01:02:21 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2010/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 2
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Feb 2019 01:43:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.

Vihang Karajgaonkar has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
---
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
D fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreNotificationNeedsInvalidateException.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M impala-parent/pom.xml
8 files changed, 1,594 insertions(+), 752 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/65/12365/8
--
To view, visit http://gerrit.cloudera.org:8080/12365
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................

IMPALA-7976 : Add a flag to disable sync using events at a table level

Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Reviewed-on: http://gerrit.cloudera.org:8080/12365
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
D fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventUtils.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
A fe/src/main/java/org/apache/impala/catalog/events/MetastoreNotificationNeedsInvalidateException.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
6 files changed, 1,651 insertions(+), 752 deletions(-)

Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 13
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/12365/8/fe/pom.xml
File fe/pom.xml:

http://gerrit.cloudera.org:8080/#/c/12365/8/fe/pom.xml@509
PS8, Line 509:     </dependency>
Mockito is often a poor choice for mocking when we own the code. Is is really needed?


http://gerrit.cloudera.org:8080/#/c/12365/8/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/12365/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@744
PS8, Line 744:       throws TException, CatalogException {
Thanks for adding these tests!


http://gerrit.cloudera.org:8080/#/c/12365/8/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@815
PS8, Line 815:         .thenReturn(dbFlag);
Here I wonder I we can simply define a test-only catalog service subclass that does what we want. We've already got several: one used for unit tests for dummy tables, another that Bharath added for a Derby-based local HMS.

Mockito is useful at times, but using it to mock our own code tends to lead to tests that are more complex and brittle than if we can define the test-only class using plain old Java.

Use your best judgement here: leave this as is if you feel that the solution shown here is simpler than the one suggested in the above comment.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Feb 2019 22:01:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Paul Rogers (Code Review)" <ge...@cloudera.org>.

Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 11: Code-Review+2

LGTM.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 11
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 21:13:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2130/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 9
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Bharath Krishna <bh...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Feb 2019 00:14:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7976 : Add a flag to disable sync using events at a table level

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12365 )

Change subject: IMPALA-7976 : Add a flag to disable sync using events at a table level
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2082/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If116edba51bae139bc003be858b3214c3f0104e3
Gerrit-Change-Number: 12365
Gerrit-PatchSet: 7
Gerrit-Owner: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (453)
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Feb 2019 03:46:42 +0000
Gerrit-HasComments: No