impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5259: Add REFRESH FUNCTIONS <db> statement
Date Thu, 18 May 2017 21:04:39 GMT
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message