You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org> on 2018/01/20 23:39:12 UTC
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9089
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 160 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/1
--
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: newchange
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 4: Code-Review+1
carry the +1 from Bikram.
any takers to +2 this?
--
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: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Jan 2018 01:29:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1819/
--
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: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:09:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h@49
PS6, Line 49: When the caller requests a ptr into the library, they
: /// are given the entry handle and must decrement the ref count when they
: /// are done.
> maybe clarify this to indicate that a handle's scope takes care of that?
updated to reflect the current state
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h@79
PS6, Line 79: /// '*entry' is expected to be null. The caller must call DecrementUseCount(*entry)
: /// when it is done using 'path'.
> that needs updating.
Done
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h@185
PS6, Line 185: set_entry
> since not strictly a "setter", probably should be SetEntry().
Done
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.cc@194
PS6, Line 194: (
> remove extraneous parens
Done
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.cc@196
PS6, Line 196: (
> same
Done
--
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: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 23:35:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 2:
(1 comment)
I think the usage LibCacheEntry* interface is analogous to the existing GetSoFunctionPtr() interface. I think in a lot of other places we've exposed handles that automatically close or DCHECK wrapping the internal data structure. I agree that the alternative approach is less error-prone so is worth considering. I don't feel strongly about whether to do it in this patch or defer the work (the use ScopeExitTrigger pattern solves the problem but is a bit clunky).
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
Isn't LibCacheEntry shared between all of the callers?
--
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 <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 01:56:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
LGTM unless Dan feels strongly about bundling the cleanup into this commit.
http://gerrit.cloudera.org:8080/#/c/9089/5/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/5/be/src/runtime/lib-cache.h@78
PS5, Line 78: /// '*entry' is expected to be null. The caller must call DecrementUseCount(*entry)
: /// when it is done using 'path'.
> is the 'entry' opaque to the caller and only used to decrement the referenc
Vuk filed IMPALA-6439 to do that cleanup.
--
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: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 21:50:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 8: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1819/
--
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: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 04:03:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9089/7/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/7/be/src/runtime/lib-cache.h@83
PS7, Line 83: must held
> garbled. Maybe: 'handle' must remain in scope while 'path' is used?
whoops, missed a word there. fixed.
--
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: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:06:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9089/7/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/7/be/src/runtime/lib-cache.h@83
PS7, Line 83: must held
garbled. Maybe: 'handle' must remain in scope while 'path' is used?
--
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: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:02:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 5:
next change replaces the explicitly managed opaque cache entry with a handle that decrements the ref-count in the destructor. makes the client side easier to use/less error-prone.
--
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: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 23:00:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#8).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 171 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/8
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 3:
(1 comment)
jira for further cleanup: IMPALA-6439
http://gerrit.cloudera.org:8080/#/c/9089/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/9089/3/tests/query_test/test_udfs.py@440
PS3, Line 440:
> nit: extra space at the end, look for red boxes
whoops.. missed these.
--
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: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 20:27:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 8: Code-Review+2
--
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: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:09:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#9).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 172 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/9
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#3).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 167 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/3
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#4).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 167 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/4
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 9: Verified+1
--
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: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 20:00:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1824/
--
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: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 16:22:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 8:
clang tidy found an issue.. update fixes it and what seems to be another example of the same issue in existing code.
--
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: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 05:44:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 9: Code-Review+2
--
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: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 16:22:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 01:39:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 160 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/2
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#7).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 171 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/7
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Reviewed-on: http://gerrit.cloudera.org:8080/9089
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 172 insertions(+), 53 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 10
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#6).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 166 insertions(+), 51 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/6
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/runtime/lib-cache.h@78
PS4, Line 78: entry is non-null
I don't see any callers where entry is not provided - can we remove? Would be nice to avoid that pattern since it's prone to races.
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/service/fe-support.cc@299
PS4, Line 299: Status status = LibCache::instance()->GetLocalPath(
Should this use the ScopeExitTrigger pattern? I guess it's not necessary but might be better to be consistent. I'll leave it up to you.
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/service/fe-support.cc@400
PS4, Line 400: LibCache::instance()->DecrementUseCount(entry);
Same question here.
--
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: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 16:40:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 6: Code-Review+2
(5 comments)
Thanks, this looks like another nice step in the right direction.
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h@49
PS6, Line 49: When the caller requests a ptr into the library, they
: /// are given the entry handle and must decrement the ref count when they
: /// are done.
maybe clarify this to indicate that a handle's scope takes care of that?
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h@79
PS6, Line 79: /// '*entry' is expected to be null. The caller must call DecrementUseCount(*entry)
: /// when it is done using 'path'.
that needs updating.
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.h@185
PS6, Line 185: set_entry
since not strictly a "setter", probably should be SetEntry().
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.cc@194
PS6, Line 194: (
remove extraneous parens
http://gerrit.cloudera.org:8080/#/c/9089/6/be/src/runtime/lib-cache.cc@196
PS6, Line 196: (
same
--
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: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 23:13:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9089
to look at the new patch set (#5).
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
IMPALA-6215: Removes race when using LibCache.
LibCache's api to provide access to locally cached files has a race.
Currently, the client of the cache accesses the locally cached path
as a string, but nothing guarantees that the associated file is not
removed before the client is done using it. This race is suspected
as the root cause for the flakiness seen in IMPALA-6092. These tests
fail once in a while with classloader errors unable to load java udf
classes. In these tests, the lib cache makes no guarantee that the path
to the jar will remain valid from the time the path is acquired through
the time needed to fetch the jar and resolve the needed classes.
LibCache offers liveness guarantees for shared objects via reference
counting. The fix in this patch extends this API to also cover paths
to locally cached files.
Testing:
- added a test to test_udfs.py that does many concurrent udf uses and
removals. By increasing the concurrent operations to 100, the issue
in IMPALA-6092 is locally reproducible on every run. With this fix,
the problem is no longer reproducible with this test.
Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
---
M be/src/codegen/llvm-codegen.cc
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M tests/query_test/test_udfs.py
8 files changed, 156 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/9089/5
--
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: newpatchset
Gerrit-Change-Id: I9175085201fe8b11424ab8f88d7b992cb7b0daea
Gerrit-Change-Number: 9089
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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)
Absolutely agree that lib-cache api is bit clunky. Main thing with this patch is the fix so that we can run the tests as before (I've reverted my previous change in this patch). I've kept the api the same as GetSoFunctionPtr for uniformity but agree that this should be simplified. I'll file a JIRA-- what's your favorite current handle api in the code base for reference?
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);}
: );
> Isn't LibCacheEntry shared between all of the callers?
yes, it's shared between callers (same deal with the GetSoFunctionPtr)
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
Done
--
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 <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 02:56:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9089/5/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/5/be/src/runtime/lib-cache.h@78
PS5, Line 78: /// '*entry' is expected to be null. The caller must call DecrementUseCount(*entry)
: /// when it is done using 'path'.
is the 'entry' opaque to the caller and only used to decrement the reference count? if so, maybe we should make this parameter be a handle class that's effectively the scope exit thing and handles the reference counting for us. That way, a callsite can't forget to decrement, and each callsite doesn't have to define the same scope exit handler code itself.
--
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: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 21:45:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/runtime/lib-cache.h@78
PS4, Line 78: entry is non-null
> I don't see any callers where entry is not provided - can we remove? Would
yup, removed. I first went for consistency with the other one (GetSoFunctionPtr) but since the plan is to change both of these, I went ahead and simplified this one.
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/service/fe-support.cc@299
PS4, Line 299: Status status = LibCache::instance()->GetLocalPath(
> Should this use the ScopeExitTrigger pattern? I guess it's not necessary bu
right, I left it off since the current code did not need it.
but that's a fair point about consistency and will help if anyone changes this area in the future, so I've added it.
http://gerrit.cloudera.org:8080/#/c/9089/4/be/src/service/fe-support.cc@400
PS4, Line 400: LibCache::instance()->DecrementUseCount(entry);
> Same question here.
didn't need it, but added it for consistency.
--
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: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 21:39:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9089 )
Change subject: IMPALA-6215: Removes race when using LibCache.
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9089/5/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9089/5/be/src/runtime/lib-cache.h@78
PS5, Line 78: /// '*entry' is expected to be null. The caller must call DecrementUseCount(*entry)
: /// when it is done using 'path'.
> Vuk filed IMPALA-6439 to do that cleanup.
after discussing a bit, how about this: I'll add a handle, but keep the cache entry members opaque for now. that will get rid of the explicit scope-exit pattern. I'll clarify IMPALA-6439 to be specific about upgrading the function ptr api and exposing the members of the cache entry properly.
--
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: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 22:05:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6215: Removes race when using LibCache.
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+1
(2 comments)
> (2 comments)
>
> Absolutely agree that lib-cache api is bit clunky. Main thing with
> this patch is the fix so that we can run the tests as before (I've
> reverted my previous change in this patch). I've kept the api the
> same as GetSoFunctionPtr for uniformity but agree that this should
> be simplified. I'll file a JIRA-- what's your favorite current
> handle api in the code base for reference?
Sounds good. I cant think of a handle API off the top of my head that would be suitable for this, but please go ahead and file the 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);}
: );
> Isn't LibCacheEntry shared between all of the callers?
oh yea thats right.
http://gerrit.cloudera.org:8080/#/c/9089/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/9089/3/tests/query_test/test_udfs.py@440
PS3, Line 440:
nit: extra space at the end, look for red boxes
--
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: 3
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:41:34 +0000
Gerrit-HasComments: Yes