Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12428 )
Change subject: IMPALA-7961: Avoid adding unmodified objects to DDL response ...................................................................... Patch Set 3: (5 comments) Initial, superficial review. http://gerrit.cloudera.org:8080/#/c/12428/3/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/12428/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2243 PS3, Line 2243: FeCatalogUtils.debugString(result.removed_catalog_objects))); Nit: There is a trick you can use here. The logger has its own (clunky) formatting to avoid the need to format a string that we typically throw away. LOG.error("Couldn't retrieve .. for {} ... Updated objects: {}, deleted objects: {}", args); Or, since the called functions are rather complex, might be easier to leave the code as it is, but enclose it in an if-statement that checks for debug level. http://gerrit.cloudera.org:8080/#/c/12428/3/fe/src/main/java/org/apache/impala/catalog/TopicUpdateLog.java File fe/src/main/java/org/apache/impala/catalog/TopicUpdateLog.java: http://gerrit.cloudera.org:8080/#/c/12428/3/fe/src/main/java/org/apache/impala/catalog/TopicUpdateLog.java@107 PS3, Line 107: LOG.info(String.format("Topic update log GC started. GC-ing topics with versions " + Here you can use the formatting trick mentioned previously. http://gerrit.cloudera.org:8080/#/c/12428/3/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/12428/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1765 PS3, Line 1765: * Returns 'true' if a new table has been created with the given params, false Nit: @return true if ... http://gerrit.cloudera.org:8080/#/c/12428/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1766 PS3, Line 1766: * otherwise. Might be useful to explain the response. I see a comment saying we are not adding an existing table. But, it is not immediately clear why that is or is not a good thing. What is the response supposed to represent? How will it be used? What guarantees does this method make about the response? http://gerrit.cloudera.org:8080/#/c/12428/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1782 PS3, Line 1782: } Nit: Using the log formatting can avoid the if-statement -- To view, visit http://gerrit.cloudera.org:8080/12428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If3e914b70ba796c9b224e9dea559b8c40aa25d83 Gerrit-Change-Number: 12428 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Mon, 11 Feb 2019 19:27:54 +0000 Gerrit-HasComments: Yes
