impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/catalog/

Line 622:       org.apache.hadoop.hive.metastore.api.Database msDb =
> // Contains native functions in its params map.

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 

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 

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.,

Line 666:     } catch (Exception e) {
> I think this error should be propagated as well. If we hit this exception, 
File fe/src/main/java/org/apache/impala/catalog/

Line 371:   public void removeAllFunctions() {
> synchronized (functions_) ?
File fe/src/main/java/org/apache/impala/catalog/

Line 258:         removeFunction(Function.fromThrift(catalogObject.getFn()));
> It's wrong to unconditionally remove the function here because the catalogO
File fe/src/main/java/org/apache/impala/service/

Line 3095:       // If Db_name is set, this is a "refresh functions" operation.
> Shrink to: This is a "refresh functions" operation.
File tests/custom_cluster/

Line 243:       assert len(glob.glob(self.LOCAL_LIBRARY_DIR + "/*.jar")) == 0
> Add this to the bottom of your new tests as well

Line 254:     by setting DBPROPERTIES of a database.'''
> Nice!

Line 333:     # Drop the original function and create a different function with the same name
> ... but a different jar as the original.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I3625c88bb51cca833f3293c224d3f0feb00e6e0b
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message