Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5259: Add REFRESH FUNCTIONS <db> statement ......................................................................
Patch Set 2: (6 comments) I have a few high level comments around the usability and error reporting. Thoughts? http://gerrit.cloudera.org:8080/#/c/6878/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: PS2, Line 628: loadJavaFunctions Same for these functions, they are swallowing exceptions. Line 629: } catch (Exception e) { Looks like we are swallowing this exception and printing it to the Catalog log. Instead we should wrap it as a CatalogException and throw? Line 630: LOG.warn("Encountered an exception while refreshing functions: " + dbName + LOG.error ? Looks like an error to me. PS2, Line 639: "Database '" + dbName + "' not found") May be use the template DB_DOES_NOT_EXIST_ERROR_MSG for consistency. Also we are catching this exception below. Line 666: } catch (Exception e) { I think this error should be propagated as well. If we hit this exception, it looks like a partial update (additions/removals) of functions is possible. Is that state consistent/desired? http://gerrit.cloudera.org:8080/#/c/6878/2/fe/src/main/java/org/apache/impala/catalog/Db.java File fe/src/main/java/org/apache/impala/catalog/Db.java: Line 371: public void removeAllFunctions() { synchronized (functions_) ? -- To view, visit http://gerrit.cloudera.org:8080/6878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3625c88bb51cca833f3293c224d3f0feb00e6e0b Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
