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 <vih...@cloudera.com> Gerrit-Reviewer: Anonymous Coward (453) Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Fri, 08 Feb 2019 00:59:55 +0000 Gerrit-HasComments: Yes
