Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache. ...................................................................... Patch Set 2: (2 comments) look good to me. But the interface for lib-cache seems conceptually wrong to me. The client to a cache should not be responsible for a cache object's lifetime (keeping track of ref counts, since relying on the client for this will make it prone to bugs). A more cleaner implementation of libcache should just return a LibCacheEntry that keeps track of its refcount internally based on its lifetime and also exposes whether that entry is alive, and if not, then gives the option to refresh it. Not sure if we should fix this in this patch or file a separate JIRA. http://gerrit.cloudera.org:8080/#/c/9089/2/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/9089/2/be/src/codegen/llvm-codegen.cc@333 PS2, Line 333: MakeScopeExitTrigger([entry]() { : if (entry != nullptr) LibCache::instance()->DecrementUseCount(entry);} : ); maybe do this in LibCacheEntry's destructor and use a unique_ptr here http://gerrit.cloudera.org:8080/#/c/9089/2/tests/query_test/test_udfs.py File tests/query_test/test_udfs.py: http://gerrit.cloudera.org:8080/#/c/9089/2/tests/query_test/test_udfs.py@443 PS2, Line 443: create_fn_to_drop = "create function {0}.foo_{1}() returns string LOCATION '{2}' SYMBOL='org.apache.impala.TestUpdateUdf'" : create_fn_to_use = "create function {0}.use_it(string) returns string LOCATION '{1}' SYMBOL='org.apache.impala.TestUdf'" : drop_fn = "drop function if exists {0}.foo_{1}()" : use_fn = "select * from (select max(int_col) from functional.alltypesagg where {0}.use_it(string_col) = 'blah' union all (select max(int_col) from functional.alltypesagg where {0}.use_it(String_col) > '1' union all (select max(int_col) from functional.alltypesagg where {0}.use_it(string_col) > '1'))) v" : nit: long lines -- To view, visit http://gerrit.cloudera.org:8080/9089 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea Gerrit-Change-Number: 9089 Gerrit-PatchSet: 2 Gerrit-Owner: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Comment-Date: Wed, 24 Jan 2018 01:39:59 +0000 Gerrit-HasComments: Yes
