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
