Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-5259: Add REFRESH FUNCTIONS <db> statement ......................................................................
Patch Set 2: (14 comments) 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: Line 622: org.apache.hadoop.hive.metastore.api.Database msDb = > // Contains native functions in its params map. Done PS2, Line 628: loadJavaFunctions > Same for these functions, they are swallowing exceptions. Are you suggesting to modify the functions and have them throw an exception? This will change the current behavior quite significantly. Do we really want this? Line 629: } catch (Exception e) { > Looks like we are swallowing this exception and printing it to the Catalog Done Line 630: LOG.warn("Encountered an exception while refreshing functions: " + dbName + > LOG.error ? Looks like an error to me. Replaced this with throwing an exception. PS2, Line 639: "Database '" + dbName + "' not found") > May be use the template DB_DOES_NOT_EXIST_ERROR_MSG for consistency. Also Done PS2, Line 639: "Database '" + dbName + "' not found") > Also move the try below this, otherwise you will just catch and swallow it. Done. I thought that was the idea, to throw the exception and catch it below. Line 655: // was dropped and a different function with the same name and signature was > Mention what could be different about this new function from the HMS, e.g., Done Line 666: } catch (Exception e) { > I think this error should be propagated as well. If we hit this exception, Done 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_) ? Done http://gerrit.cloudera.org:8080/#/c/6878/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: Line 258: removeFunction(Function.fromThrift(catalogObject.getFn())); > It's wrong to unconditionally remove the function here because the catalogO Done http://gerrit.cloudera.org:8080/#/c/6878/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: Line 3095: // If Db_name is set, this is a "refresh functions" operation. > Shrink to: This is a "refresh functions" operation. Done http://gerrit.cloudera.org:8080/#/c/6878/2/tests/custom_cluster/test_permanent_udfs.py File tests/custom_cluster/test_permanent_udfs.py: Line 243: assert len(glob.glob(self.LOCAL_LIBRARY_DIR + "/*.jar")) == 0 > Add this to the bottom of your new tests as well Done Line 254: by setting DBPROPERTIES of a database.''' > Nice! Thanks! Line 333: # Drop the original function and create a different function with the same name > ... but a different jar as the original. Done -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
