Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19378 )
Change subject: IMPALA:11808: Added support for reload event in catalogD ...................................................................... Patch Set 2: (20 comments) The solution looks good to me. I'm going to take a deeper look. http://gerrit.cloudera.org:8080/#/c/19378/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19378/2//COMMIT_MSG@7 PS2, Line 7: IMPALA:11808: Added nit: "IMPALA-11808: Add ..." http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java File fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java: http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/compat-hive-3/java/org/apache/impala/compat/MetastoreShim.java@499 PS2, Line 499: fireReloadEventHelper We need the same method in another MetastoreShim: https://github.com/apache/impala/blob/master/fe/src/compat-apache-hive-3/java/org/apache/impala/compat/MetastoreShim.java BTW, could you add method comments like other methods? http://gerrit.cloudera.org:8080/#/c/19378/2/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/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2435 PS2, Line 2435: private boolean isRefresh_; Coud you comment that if isRefresh_ is false, it's an invalidate event? http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2441 PS2, Line 2441: nit: 4 spaces indent http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2444 PS2, Line 2444: ReloadMessage reloadMessage = : MetastoreEventsProcessor.getMessageDeserializer() : .getReloadMessage(event.getMessage()); : try { : msTbl_ = Preconditions.checkNotNull(reloadMessage.getTableObj()); : reloadPartition_ = reloadMessage.getPtnObj(); : isRefresh_ = reloadMessage.isRefreshEvent(); We might need to move this into MetastoreShim if Apache Hive3 doesn't have these methods. http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2462 PS2, Line 2462: nit: 4 spaces indent http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2464 PS2, Line 2464: nit: 4 spaces indent http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2468 PS2, Line 2468: nit: 4 spaces indent http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2485 PS2, Line 2485: else { nit: to keep this method clean, can we extract codes in the else-branch into a method? http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2493 PS2, Line 2493: nit: 4 spaces indent http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2501 PS2, Line 2501: {} This is always -1. nit: should be 4 spaces indent at this line http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2531 PS2, Line 2531: nit: need to fix the indention http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2545 PS2, Line 2545: false Could you add a comment that we always treat the table as non-transactional so all files are reloaded? http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@2548 PS2, Line 2548: debugString("Refresh table {} failed. Event processing " : nit: need to fix the indention http://gerrit.cloudera.org:8080/#/c/19378/2/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/19378/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6275 PS2, Line 6275: // Quick check to see if the table exists in the catalog without triggering : // a table load. nit: Please move this comment back since it's more about the null check. http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6337 PS2, Line 6337: // fire event for reload Can we extract the following code into a method? The execResetMetadata() method is too long now. I think in the future, we should also refactor execResetMetadata() to clean it up.. So I think these code should be moved into MetastoreShim since we can't run them on Apache Hive3. http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6341 PS2, Line 6341: nit: 4 spaces indent http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6354 PS2, Line 6354: tryWriteLock(tbl); The default timeout for this is 2 hours. It might not worth waiting so long for tracking self events. We can use the underlying method to wait for a shorter time, e.g. 10mins (or make this configurable so we have a way to skip it): if (req.isIs_refresh() && catalog_.tryLock(tbl, true, 600000)) { catalog_.addVersionsForInflightEvents(false, tbl, newCatalogVersion); } else { LOG.warn(...); } BTW, we don't need to add the inflight version for invalidate ops. In such case, 'tbl' is invalidated and no longer valid. http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6356 PS2, Line 6356: nit: remove space http://gerrit.cloudera.org:8080/#/c/19378/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@6358 PS2, Line 6358: refreshTable nit: Can we use a more specifit name, e.g. fireReloadEvent ? -- To view, visit http://gerrit.cloudera.org:8080/19378 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic62d58837d356dc2113f3c0904228ac9de484136 Gerrit-Change-Number: 19378 Gerrit-PatchSet: 2 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Comment-Date: Tue, 20 Dec 2022 12:35:14 +0000 Gerrit-HasComments: Yes
