Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12889 )
Change subject: IMPALA-7971: Add support for insert events in event processor. ...................................................................... Patch Set 6: (15 comments) Thanks for making the changes. The patch looks great. I have left some minor suggestions and then its good from my side. http://gerrit.cloudera.org:8080/#/c/12889/6/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/12889/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2077 PS6, Line 2077: after inserts nit, use "after insert events" to be more specific http://gerrit.cloudera.org:8080/#/c/12889/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/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@423 PS6, Line 423: refreshCatalogTable since this method is not doing anything else apart from calling catalog_refreshTableIfExists .. may be we can get rid of it. http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@431 PS6, Line 431: throws CatalogException { same as above, The method is doing nothing other than calling another method. So may as well remove it and call the underlying method directly. http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@641 PS6, Line 641: List<String> partVals = insertPartition_.getValues(); probably worth while to check the following conditions: Preconditions.checkNotNull(partVals) Preconditions.checkState (fsList.size() == partVals.size()) http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@649 PS6, Line 649: Automatic nit, remove Automatic. kind of redundant since debugLog prints the event info as well. http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@673 PS6, Line 673: Automatic nit, remove http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12889/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3580 PS5, Line 3580: rtition.isMarkedCached()) > This code path is taken by only HDFSTables as you can see on L3522. Got it. Thanks for the clarification. http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3578 PS6, Line 3578: filesBeforeInsertForPartitions.put(partition.getId(), Can we do this conditionally when event processing is enabled? http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3722 PS6, Line 3722: createInsertEventForPartition nit, Adding plural to the name makes it more readable since it is firing multiple insert events. may be, createInsertEventsForPartitions? http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3741 PS6, Line 3741: for (String fileName : deltaFiles) { : newFiles.add(fileName); : } Looks like you are making this copy because the InsertEventRequestData in fireInsertEvent method needs a list. In such a case, perhaps a cleaner way is to just pass deltaFiles from here and make the copy in that method simply using rqst.setFilesAdded(new ArrayList<>(deltaFiles)) http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3767 PS6, Line 3767: for (String fileName : deltaFiles) { : newFiles.add(fileName); : } same as previous comment, you can just pass the deltaFiles to the fireInsertEvent and reuse the logic http://gerrit.cloudera.org:8080/#/c/12889/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/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@432 PS6, Line 432: public void testInsertEvents() throws TException, ImpalaException { thanks for adding this test! http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@463 PS6, Line 463: String parentPathString = : "/test-warehouse/" + dbName +".db/" + tblName; : String filePathString = isPartitionInsert ? "/p1=testPartVal/testFile.0" : : "/testFile.0"; This could be flaky if in the future the default location for tests changes. Its probably better to use the table location instead of this. so something like msTable.getSd().getLocation() http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@476 PS6, Line 476: out.close(); do this in finally clause to avoid leaks? http://gerrit.cloudera.org:8080/#/c/12889/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@510 PS6, Line 510: size s/size/count -- To view, visit http://gerrit.cloudera.org:8080/12889 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c48c5ca4bde18d532c582980aebbc25f1bf1c52 Gerrit-Change-Number: 12889 Gerrit-PatchSet: 6 Gerrit-Owner: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Bharath Krishna <bhar...@cloudera.com> 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, 09 Apr 2019 20:44:53 +0000 Gerrit-HasComments: Yes
