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 <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: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 12 Feb 2019 00:26:56 +0000 Gerrit-HasComments: Yes
