Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9154 )

Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll 
extenion crashes impala
......................................................................


Patch Set 3:

(4 comments)

Thanks for fixing this, it would be good to get this merged. The test coverage 
looks good and the fix is valid, I just noticed the test may be flaky and the 
structure of the error handling in the code could be improved to reduce the 
chance of similar mistakes in future.

http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc@223
PS3, Line 223: CreateFromMemory
This function also has the same bad pattern of not closing 'codegen' on errors 
- we should probably fix that too while we're here.


http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc@1315
PS3, Line 1315:   Status status = CreateFromFile(nullptr, &pool, nullptr, file, 
module_id, &codegen);
CreateFromFile() should really close the LlvmCodegen object on failure - it's 
awkward requiring the caller to do it. Might be easier to use the "goto error" 
pattern with a cleanup block at the end of the CreateFromFile() function.


http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py@332
PS3, Line 332:       with open("bad_udf.ll", "w") as f:
The local and HDFS file creation will race if test_udf_errors() runs with 
itself concurrently. Would be best to use tempfile.mkstemp() for the local file 
and include the unique_database string in the path of the HDFS file.


http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py@338
PS3, Line 338: check_call
Use call() instead of check_call(), since we want to execute the next cleanup 
step even on errors.



--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 3
Gerrit-Owner: anujphadke <apha...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: anujphadke <apha...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 06:29:39 +0000
Gerrit-HasComments: Yes

Reply via email to