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: (5 comments) patch looks good to me. Just wanted to make sure that we are handling the replace flag correctly below when generating the insert events. 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 > I left this here because I wanted to add metrics of "Tables Refreshed" in a Sounds good if you plan to use it later. 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 earlier reply. Wanted to have metric for partitionsRefreshed here. sounds good. http://gerrit.cloudera.org:8080/#/c/12889/9/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/9/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3823 PS9, Line 3823: This is interesting because based on the thrift definition of InsertEventRequestData the default value of replace flag is unset (which means false since its a boolean). Perhaps we should be explicitly set it to true here as well to make sure. Suggest you to do the following. isOverwrite ? insertData.setReplace(true) : insertData.setReplace(false); 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@463 PS6, Line 463: String parentPathString = : "/test-warehouse/" + dbName +".db/" + tblName; : String filePathString = isPartitionInsert ? "/p1=testPartVal/testFile.0" : : "/testFile.0"; > You are right. However, almost all the FE tests have hardcoded this locatio I am not a big fan of hardcoding paths in the tests esp. if the root location can be trivially found by catalog_.getTable().getMetastoreTable(). You are getting the tbl object anyways down below at line 489. If you move that line here you don't need any extra calls. Not a blocker comment, can be done later if needed. 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(); > I moved it to finally. Do you know a less messy way to do this? Looks like FSDataOutputStream does not implement Autocloseable, hence try-with-resources could not be used. Guess we will have to live with this for now. -- 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: Thu, 11 Apr 2019 19:57:51 +0000 Gerrit-HasComments: Yes
