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/03/17 00:49:11 UTC
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Vuk Ercegovac has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9697
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
IMPALA-6670: refresh lib-cache entries from plan (WIP)
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
19 files changed, 265 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/4
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 10: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9697/10/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/10/fe/src/main/java/org/apache/impala/catalog/Function.java@396
PS10, Line 396: getLocation() != null &&
: getLocation().toString().length() > 0
in what case will we have no location (or zero length location) when the type is not BUILTIN?
http://gerrit.cloudera.org:8080/#/c/9697/10/fe/src/main/java/org/apache/impala/catalog/Function.java@406
PS10, Line 406: }
I think it'd be worth at least commenting here that the intention is to result in an error. Otherwise, It's not clear why MAX_VALUE would be returned given that value has no special meaning to libcache.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 10
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:49:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has removed a vote on this change.
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Removed Code-Review-2 by Alex Behm <al...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 15: Code-Review+2
(2 comments)
Let's give Dan/Dimitris another chance to review tomorrow before merging. I'm happy with this change.
http://gerrit.cloudera.org:8080/#/c/9697/15/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/15/tests/custom_cluster/test_coordinators.py@223
PS15, Line 223: time.sleep(2)
Shouldn't the sleep go before L222?
http://gerrit.cloudera.org:8080/#/c/9697/15/tests/custom_cluster/test_coordinators.py@234
PS15, Line 234: # IMPALA-6719: workaround lower-cases db-name.
lol nice one
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 15
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 06:04:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9697/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2155
PS15, Line 2155: private void checkSerializedMTime(Expr fnExpr, boolean expectMTime) {
fnExpr -> expr
http://gerrit.cloudera.org:8080/#/c/9697/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2168
PS15, Line 2168: // Checks that 'fnCall', when serialized to thrift, has mtime
fix 'fnCall'
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 15
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 06:06:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 20:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9697/20/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/20/be/src/runtime/lib-cache.cc@410
PS20, Line 410: if (status.ok() && exp_mtime >= 0 && fs_last_modified_time != exp_mtime) {
Would it be simpler to hoist this above line 404, then just set:
status = Status(TErrorCode::LIB_VERSION_MISMATCH, ...)
And let the !status.ok() body do all the work.
http://gerrit.cloudera.org:8080/#/c/9697/20/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/20/fe/src/main/java/org/apache/impala/catalog/Function.java@57
PS20, Line 57: private final static Logger LOG = LoggerFactory.getLogger(Function.class);
1 more Logger here.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 20
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 01:24:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#17).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 446 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/17
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 17
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 5:
(8 comments)
I'm not very familiar with the BE libcache, still looking at at.
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.h@640
PS5, Line 640: /// ensure that the cached hdfs_file that's used is the most recent.
We should document the behavior of special arg values like -1 or LONG_MAX
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.h@642
PS5, Line 642: const std::string& hdfs_file, const time_t last_modified_time);
Why not time_t mtime? I think that's equally expressive since mtime is a well-known term.
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc@801
PS5, Line 801: time_t mtime =
Is it valid for the last_modified_time to not be set here?
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@606
PS5, Line 606: fn_.UpdateLastModifiedTime();
Would be good to have a FE unit test:
* built-in agg/analytic/scalar functions should not have this set
* user-defined agg and scalar functions should have this set
Why are we doing this in analysis and not in toThrift()? An expr may be analyzed many times.
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/catalog/Function.java@178
PS5, Line 178: protected void setLastModifiedTime(long t) { lastModifiedTime_ = t; }
I don't think we should modify CatalogObjects during query analysis in impalads because those are shared among queries. We can have several queries analyzing the same expression, so getting/setting the mtime will be racy.
Instead, I think we should get and set the mtime in Expr.treeToThriftHelper()
There's still a chance of intra-query inconsistency, but at least there can't be a race between different queries.
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/catalog/Function.java@397
PS5, Line 397: public final void UpdateLastModifiedTime() {
confusing collection of function names that need cleanup:
getLastModifiedTime()
setLastModifiedTime()
UpdateLastModifiedTime()
GetLastModifiedTime()
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py@133
PS5, Line 133: db_name = 'TEST_EXEC_ONLY_CACHE'
use unique_database to have it cleaned up automatically
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py@203
PS5, Line 203: self.execute_query_expect_success(client,
might never happen if the test fails, use unique_database
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 05:10:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#9).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 379 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/9
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 11:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h@79
PS8, Line 79: /// exp_mtime differs from the mtime on the file system.
> If an error is returned because the mtime differs, will the cache and the l
I think you might have missed this comment.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@391
PS11, Line 391: protected TSymbolLookupParams getLookupParams() { return null; }
abstract?
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@393
PS11, Line 393: // Looks up the last time the function's source file was updated as recorded in its
What does it return if there was an error?
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@394
PS11, Line 394: // lib-cache entry. Returns -1 if a modified time is not applicable.
not necessary obvious that the lib cache is in the BE, so better "in its backend lib-cache entry"
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@399
PS11, Line 399: Preconditions.checkNotNull(lookup);
combine with previous line:
TSymbolLookupParams lookup = Preconditios.checkNotNull(getLookupParams());
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@407
PS11, Line 407: // There was an error looking up the lib. A max mtime will result in an error.
The last part is really nebulous to me. How will a max mtime result in an error? What kind of error?
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2162
PS11, Line 2162: // HACK: aggregates contain slot-refs in their child expressions. they are
Cleaner solution:
* call stmt.materializeRequiredSlots(stmt.getAnalyzer());
* check stmt.getAggInfo().getAggregateExprs().get(0) as well as stmt.getAggInfo().getMergeAggInfo().getAggregateExprs().get(0)
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2186
PS11, Line 2186: testMTime("avg(int_col) from functional.alltypes", false);
I think you can move the "from functional.alltypes" part into L2159, we have similar patterns elsewhere
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@203
PS8, Line 203:
> this proved to be difficult. my approach was to drop/create so as to modify
thanks for trying, let me think whether I can come up with a sensible approach
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 11
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 18:42:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#13).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 434 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/13
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 13
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 8:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@40
PS8, Line 40: import org.apache.impala.thrift.TSymbolType;
> revert imports?
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@206
PS8, Line 206: protected long GetLastModifiedTimeInternal() {
> getLastModifiedTime()?
changed it to start with a lower-case.
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@391
PS8, Line 391: // Looks up the last time the function's source file was updated. Returns -1 if a
> Comment is not accurate. We're looking up the mtime from the BE's library c
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@394
PS8, Line 394: public final long GetLastModifiedTime() {
> getCachedLastModifiedTime
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@404
PS8, Line 404: protected long GetLastModifiedTimeInternal() { return Long.MAX_VALUE; }
> Would be nice to have have the JNI invocation and exception handling in a s
yup, simplified
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2171
PS8, Line 2171: testMTime("sleep(1)", false);
> sleep() is an oddball, let's try a few more common built-ins including Expr
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2180
PS8, Line 2180: catalog_.addFunction(ScalarFunction.createForTesting("default", "udf_builtin_bug",
> I don't think we've covered the UDA case
added coverage for uda's. I think the way I dig the fn out is pretty kludgy for aggregates so let me know if there's a better way.
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@137
PS8, Line 137: os.environ['IMPALA_HOME'], 'testdata/udfs/impala-hive-udfs.jar')
> I think we generally indent 4 here (and other cases below)
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@145
PS8, Line 145: '/test-warehouse/{0}.db'.format(db_name)])
> Might need get_fs_path() here
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@203
PS8, Line 203:
> Would be good to have a test that triggers the new error and check that run
this proved to be difficult. my approach was to drop/create so as to modify the coord cache, then copy a new jar, then run a query. the reason this does not result in a bug is that add fn modifies the cache entry to refresh so that when the query is compiled, it picks up the new jar, and all works.
that said, I'm working on adding tests with multiple coordinators and queries that don't run on all executors since there are cases there to double check.
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@206
PS8, Line 206: self.execute_query_expect_success(client,
> add IF EXISTS (the database may not have been created)
Done
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@208
PS8, Line 208: client.close()
> client may not have been set yet
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 05:40:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#16).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 446 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/16
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 16
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 16: Code-Review+2
(1 comment)
Dan, Dimitris, any more comments?
http://gerrit.cloudera.org:8080/#/c/9697/16/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/16/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2169
PS16, Line 2169: // set as expected. 'fnCall' must be a single function call.
fnCall
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 16
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 17:11:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 22: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 22
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 00:50:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/16/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/16/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2169
PS16, Line 2169: // set as expected. 'fnCall' must be a single function call.
> fnCall
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 16
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 17:14:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 8:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h@79
PS8, Line 79: /// exp_mtime differs from the mtime on the file system.
If an error is returned because the mtime differs, will the cache and the local file be modified or will they be left in their current state?
Would also be good to explain why we chose that behavior.
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@40
PS8, Line 40: import org.apache.impala.thrift.TSymbolType;
revert imports?
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@206
PS8, Line 206: protected long GetLastModifiedTimeInternal() {
getLastModifiedTime()?
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@391
PS8, Line 391: // Looks up the last time the function's source file was updated. Returns -1 if a
Comment is not accurate. We're looking up the mtime from the BE's library cache which could be older than the real mtime on HDFS.
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@394
PS8, Line 394: public final long GetLastModifiedTime() {
getCachedLastModifiedTime
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/main/java/org/apache/impala/catalog/Function.java@404
PS8, Line 404: protected long GetLastModifiedTimeInternal() { return Long.MAX_VALUE; }
Would be nice to have have the JNI invocation and exception handling in a single place, and not duplicated in the overrides.
Maybe this can be done with a getLookupParams() that subclasses override, or alternatively have different implementations of buidLookupParams() directly
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2171
PS8, Line 2171: testMTime("sleep(1)", false);
sleep() is an oddball, let's try a few more common built-ins including Exprs that are not FunctionCallExprs
http://gerrit.cloudera.org:8080/#/c/9697/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2180
PS8, Line 2180: catalog_.addFunction(ScalarFunction.createForTesting("default", "udf_builtin_bug",
I don't think we've covered the UDA case
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@137
PS8, Line 137: os.environ['IMPALA_HOME'], 'testdata/udfs/impala-hive-udfs.jar')
I think we generally indent 4 here (and other cases below)
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@145
PS8, Line 145: '/test-warehouse/{0}.db'.format(db_name)])
Might need get_fs_path() here
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@203
PS8, Line 203:
Would be good to have a test that triggers the new error and check that running refresh functions indeed fixes the problem.
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@206
PS8, Line 206: self.execute_query_expect_success(client,
add IF EXISTS (the database may not have been created)
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@208
PS8, Line 208: client.close()
client may not have been set yet
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 23:04:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@77
PS7, Line 77: /// Returns an error if 'hdfs_lib_file' cannot be copied to the local fs.
I think this should say something about returning an error if the file's mtime doesn't match exp_mtime.
Also, I think you sometimes pass in -1. Comment what that means (don't check mtime?)
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@100
PS7, Line 100: /// If the entry is already cached, if its last modified time is older than
: /// expected mtime, 'exp_mtime', the entry is refreshed.
same
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@166
PS7, Line 166: /// 'exp_mtime' argument.
comment mtime error case
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@175
PS7, Line 175: /// No locks are assumed held; 'entry' should be visible only to a single thread.
mtime error case
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.cc@202
PS7, Line 202: if (entry != nullptr) *mtime = entry->last_mod_time;
I think now that you pass in entry != nullptr, you need to decrement the use_count. How can we can this bug in tests?
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.cc@404
PS7, Line 404: // Check that the expected last_modified_time is the same as what's on the filesystem.
: if (status.ok() && exp_mtime >= 0 && fs_last_modified_time != exp_mtime) {
: return Status(TErrorCode::LIB_VERSION_MISMATCH, hdfs_lib_file,
: fs_last_modified_time, exp_mtime);
: }
won't the check in LoadCacheEntry() handle that? I guess it doesn't hurt, but seems slightly more confusing to have redundancy.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 16:37:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 17: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/runtime/lib-cache.cc@202
PS17, Line 202: if (entry != nullptr) {
According to the header comment, this function returns Status.ok() iff it sets *mtime. So, shouldn't that be DCHECK(entry != nullptr), or do we need to set *mtime in the case when entry == nullptr yet returning status ok (but what does that case mean)?
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 17
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:05:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#10).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 378 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/10
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 10
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 5:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.h@640
PS5, Line 640: /// ensure that the cached hdfs_file that's used is the most recent.
> We should document the behavior of special arg values like -1 or LONG_MAX
documented in fe Function.java
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.h@642
PS5, Line 642: const std::string& hdfs_file, const time_t last_modified_time);
> Why not time_t mtime? I think that's equally expressive since mtime is a we
done (for all c++ uses)
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc@801
PS5, Line 801: time_t mtime =
> Is it valid for the last_modified_time to not be set here?
its not set for builtins, which afaict, also use this code path.
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@606
PS5, Line 606: fn_.UpdateLastModifiedTime();
> Would be good to have a FE unit test:
good point about the placement of this method. it seemed like a cleaner place to put it, but yes, since its part of a shared object, and meant just for the plan, the toThrift option is better.
wasn't clear on how to test this: get the plan, get the thrift, check the thrift setting? also, unclear on whether that's useful to test now that its moved to thrift (vs updating the function state).
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/catalog/Function.java@178
PS5, Line 178: protected void setLastModifiedTime(long t) { lastModifiedTime_ = t; }
> I don't think we should modify CatalogObjects during query analysis in impa
good point, moved.
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/catalog/Function.java@397
PS5, Line 397: public final void UpdateLastModifiedTime() {
> confusing collection of function names that need cleanup:
removed some of these.
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py@133
PS5, Line 133: db_name = 'TEST_EXEC_ONLY_CACHE'
> use unique_database to have it cleaned up automatically
didn't work for custom cluster tests. I seem to get two tests instead of one when I try it, and the second one get tripped up in cleanup (perhaps bc some super class here is shutting the cluster before the unique db cleanup?).
I noted that the other tests in this python script resorted to not use unique_database either-- perhaps for the same issue I saw. However, I also saw that other custom cluster tests do use unique_database, but it was not clear what I need to do differently here.
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py@203
PS5, Line 203: self.execute_query_expect_success(client,
> might never happen if the test fails, use unique_database
yup, see comment above about why I went this route. If I can figure out how to use it, I can fix the other tests here.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 06:56:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 15:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9697/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2155
PS15, Line 2155: private void checkSerializedMTime(Expr fnExpr, boolean expectMTime) {
> fnExpr -> expr
Done
http://gerrit.cloudera.org:8080/#/c/9697/15/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2168
PS15, Line 2168: // Checks that 'fnCall', when serialized to thrift, has mtime
> fix 'fnCall'
Done
http://gerrit.cloudera.org:8080/#/c/9697/15/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/15/tests/custom_cluster/test_coordinators.py@223
PS15, Line 223: time.sleep(2)
> Shouldn't the sleep go before L222?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 15
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 16:35:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 22: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 22
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 04:38:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9697/10/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/10/fe/src/main/java/org/apache/impala/catalog/Function.java@396
PS10, Line 396: getLocation() != null &&
: getLocation().toString().length() > 0
> in what case will we have no location (or zero length location) when the ty
looking at HdfsUri, following analysis, the location_, if its set, should never be empty. changing this to a precondition.
I could not find a place where we assert that non-builtins must have non-null locations. For now, I'll leave this additional check in.
http://gerrit.cloudera.org:8080/#/c/9697/10/fe/src/main/java/org/apache/impala/catalog/Function.java@406
PS10, Line 406: }
> I think it'd be worth at least commenting here that the intention is to res
makes sense, done.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 10
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 17:51:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19: Code-Review-2
Setting to -2 to prevent auto-merge while we examine the remaining comments
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 23:32:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
IMPALA-6670: refresh lib-cache entries from plan (WIP)
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
21 files changed, 319 insertions(+), 75 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/5
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 12:
(3 comments)
I also added a test to catch the mismatch issue.
What made it difficult to test is that symbol resolution was *always* refreshing. That's fine for add/drop fn, but unintended for a quick cache lookup when planning. I've parameterized symbol resolution based on these two cases so that does not add more trips to the fs, as intended, and the test was straightforward.
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h@79
PS8, Line 79: /// exp_mtime differs from the mtime on the file system.
> So is it fair to say that if a non-OK status is returned, the cache will be
Done
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@407
PS11, Line 407: return result.last_modified_time;
> I agree with your sentiment here. I think we should bubble up the exception
good. I'm throwing it here now. I didn't see the need for further wrapping at the caller but let me know if I've missed something.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2162
PS11, Line 2162: // not materialized at this point, so serialization will fail. clear them so
> You can fix the hack with this (I tried it):
that did it (and needed for the tuple descs in agg info). thx!
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 12
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 01:56:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2160/
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 20:07:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 11:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@391
PS11, Line 391: protected TSymbolLookupParams getLookupParams() { return null; }
> abstract?
that would make the class abstract, which I tried to avoid so I've replaced this method to throw an exception (I've seen other places that do this).
looking more at this, I'm unclear why this is not abstract. for example the thrift for TFunction states that one of tscalar or tagg must be set (is that stale?). Looking at the places that do construct (tests, drop fn, several types of builtins, e.g, "in"), I suspect that these could be handled differently-- but that sort of change should be handled separately from this one.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@393
PS11, Line 393: // Looks up the last time the function's source file was updated as recorded in its
> What does it return if there was an error?
Done
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@394
PS11, Line 394: // lib-cache entry. Returns -1 if a modified time is not applicable.
> not necessary obvious that the lib cache is in the BE, so better "in its ba
Done
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@399
PS11, Line 399: Preconditions.checkNotNull(lookup);
> combine with previous line:
Done
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@407
PS11, Line 407: // There was an error looking up the lib. A max mtime will result in an error.
> The last part is really nebulous to me. How will a max mtime result in an e
yeah, I don't like returning from here. I'd prefer to throw an exception and fail early but am unclear about error handling for when this method is called. as expected, toThrift should not fail.
for now, I've updated the comments.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2162
PS11, Line 2162: // HACK: aggregates contain slot-refs in their child expressions. they are
> Cleaner solution:
I tried materializeRequiredSlots before but, for example, the avg has a cast expression, with a slot ref that does not get materialized, so fails serialization. Is there some other method(s) that the planner would have made to have this work? Perhaps this should be a planner test since its asserting properties about the plan?
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2186
PS11, Line 2186: testMTime("avg(int_col) from functional.alltypes", false);
> I think you can move the "from functional.alltypes" part into L2159, we hav
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 11
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 20:11:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc@411
PS19, Line 411: return Status(TErrorCode::LIB_VERSION_MISMATCH, hdfs_lib_file,
> Should you also RemoveEntryInternal in this case?
Vuk and I discussed this and I think this would be the right thing to do in the event of a backwards leap of mtime; such a thing would be extremely unusual, but could happen in the event the HDFS namenode gets restored from a backup or fails and is manually replaced with a spare that has stale data.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 00:40:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#8).
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
IMPALA-6670: refresh lib-cache entries from plan (WIP)
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
23 files changed, 374 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/8
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 8
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 14:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9697/14/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/9697/14/common/thrift/Frontend.thrift@644
PS14, Line 644: 8: required bool use_cache
> Why not needs_refresh or force_refresh to be consistent with the refresh te
makes sense, went with needs_refresh
http://gerrit.cloudera.org:8080/#/c/9697/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2168
PS14, Line 2168: // Checks that 'fnCall', when serialized to thrift, has mtime
> Let's call 'fnCall' 'expr' for consistency. By 'fnCall' we typically mean F
Done
http://gerrit.cloudera.org:8080/#/c/9697/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2178
PS14, Line 2178: TupleDescriptor int_desc = stmt.getAggInfo().intermediateTupleDesc_;
> intDesc
Done
http://gerrit.cloudera.org:8080/#/c/9697/14/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/14/tests/custom_cluster/test_coordinators.py@221
PS14, Line 221: check_call(["hadoop", "fs", "-put", "-f", new_src_path, tgt_path_2])
> should we sleep a little before this to make sure we get a nte mtime? not s
good point... I thought I saw somewhere that it was pretty coarse
(hdfsFileInfo claims second granularity)
http://gerrit.cloudera.org:8080/#/c/9697/14/tests/custom_cluster/test_coordinators.py@228
PS14, Line 228: self.execute_query_expect_failure(client, mismatch_query)
> * Also check the error message
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 14
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 05:42:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 23:53:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc@801
PS5, Line 801: fn.__isset.last_modified_time ? fn.last_modified_time : numeric_limits<long>::max();
> Got it. Perhaps it's simpler and less bug prone to have a special value lik
it does simplify things, so changed it.
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@77
PS7, Line 77: /// Returns an error if 'hdfs_lib_file' cannot be copied to the local fs.
> I think this should say something about returning an error if the file's mt
Done
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@100
PS7, Line 100: /// If the entry is already cached, if its last modified time is older than
: /// expected mtime, 'exp_mtime', the entry is refreshed.
> same
Done
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@166
PS7, Line 166: /// 'exp_mtime' argument.
> comment mtime error case
Done
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.h@175
PS7, Line 175: /// No locks are assumed held; 'entry' should be visible only to a single thread.
> mtime error case
Done
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.cc@202
PS7, Line 202: if (entry != nullptr) *mtime = entry->last_mod_time;
> I think now that you pass in entry != nullptr, you need to decrement the us
good catch. I would have expected the dchecks in the destructors, which assert that use-count of the entry in the table == 0, to catch this. however, we remove cache entries from the table and may not delete them, so I suspect that without decrementing, we're leaking these. which testing config catches such leaks?
http://gerrit.cloudera.org:8080/#/c/9697/7/be/src/runtime/lib-cache.cc@404
PS7, Line 404: // Check that the expected last_modified_time is the same as what's on the filesystem.
: if (status.ok() && exp_mtime >= 0 && fs_last_modified_time != exp_mtime) {
: return Status(TErrorCode::LIB_VERSION_MISMATCH, hdfs_lib_file,
: fs_last_modified_time, exp_mtime);
: }
> won't the check in LoadCacheEntry() handle that? I guess it doesn't hurt, b
yes, it will cover it. decided to keep this in to cut out of loading work early.
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@606
PS5, Line 606:
> I guess checking for a specific expected value does not make much sense, bu
Done
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py@133
PS5, Line 133: db_name = 'TEST_EXEC_ONLY_CACHE'
> Hm, weird I'm not familiar with this issue. Happy to defer his cleanup, but
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 19:48:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@606
PS5, Line 606: fn_.UpdateLastModifiedTime();
> Such a FE unit test could look something like this:
I guess checking for a specific expected value does not make much sense, but we could check whether the time is set or not,and if it's set we expect it to have a value >= 0
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 16:03:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 21:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2164/
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 03:26:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/exec/external-data-source-executor.cc
File be/src/exec/external-data-source-executor.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/exec/external-data-source-executor.cc@140
PS19, Line 140: // TODO: pass the mtime from the coordinator. for now, skip the mtime check (-1).
> Please file a JIRA for this and note it in the comment.
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.h@174
PS19, Line 174: /// The entry is refreshed if it needs_refresh is set and its mtime is
> 'if it' -> 'if'
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc@411
PS19, Line 411: return Status(TErrorCode::LIB_VERSION_MISMATCH, hdfs_lib_file,
> Vuk and I discussed this and I think this would be the right thing to do in
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc@411
PS19, Line 411: return Status(TErrorCode::LIB_VERSION_MISMATCH, hdfs_lib_file,
> Should you also RemoveEntryInternal in this case?
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/service/fe-support.cc@401
PS19, Line 401: // TODO: used for external data sources; add proper mtime.
> Same comment for JIRA
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/analysis/Expr.java@615
PS19, Line 615: thriftFn.setLast_modified_time(fn_.getLastModifiedTime());
> If doing this unconditionally, why not just do this in Function's toThrift(
my I understanding is that this will get run for queries, whereas functions may be serialized for queries and other reasons such as catalog objects. since mtime is used just for the query side of things, I've kept it here.
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java:
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@40
PS19, Line 40: private final static Logger LOG = LoggerFactory.getLogger(AggregateFunction.class);
> Appears unused - is this left over from testing? If so, delete the imports
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
File fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java:
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java@47
PS19, Line 47: private final static Logger LOG = LoggerFactory.getLogger(ScalarFunction.class);
> Either I'm confused about how this gets used, or I'm seeing a lot of extra
Done
http://gerrit.cloudera.org:8080/#/c/9697/19/testdata/bin/copy-udfs-udas.sh
File testdata/bin/copy-udfs-udas.sh:
http://gerrit.cloudera.org:8080/#/c/9697/19/testdata/bin/copy-udfs-udas.sh@64
PS19, Line 64: > src/main/java/org/apache/impala/NewReplaceStringUdf.java
> This is a pretty ugly hack; does this mean you can't run the test directly
that's right-- you need to run this script first, then can run the test. would be good to clean this up, but lets leave such clean-ups out of this change.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 01:16:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/exec/external-data-source-executor.cc
File be/src/exec/external-data-source-executor.cc:
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/exec/external-data-source-executor.cc@140
PS17, Line 140: // TODO: pass the mtime from the coordinator. for now, skip the mtime check (-1).
Is there a JIRA for this? If not, please create one and reference it here.
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/exec/external-data-source-executor.cc
File be/src/exec/external-data-source-executor.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/exec/external-data-source-executor.cc@140
PS19, Line 140: // TODO: pass the mtime from the coordinator. for now, skip the mtime check (-1).
Please file a JIRA for this and note it in the comment.
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.h@174
PS19, Line 174: /// The entry is refreshed if it needs_refresh is set and its mtime is
'if it' -> 'if'
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/runtime/lib-cache.h@172
PS17, Line 172: /// need to be refreshed.
'if it' -> 'if'
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/runtime/lib-cache.cc@411
PS19, Line 411: return Status(TErrorCode::LIB_VERSION_MISMATCH, hdfs_lib_file,
Should you also RemoveEntryInternal in this case?
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/9697/19/be/src/service/fe-support.cc@401
PS19, Line 401: // TODO: used for external data sources; add proper mtime.
Same comment for JIRA
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/analysis/Expr.java@615
PS19, Line 615: thriftFn.setLast_modified_time(fn_.getLastModifiedTime());
If doing this unconditionally, why not just do this in Function's toThrift() method?
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java:
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@40
PS19, Line 40: private final static Logger LOG = LoggerFactory.getLogger(AggregateFunction.class);
Appears unused - is this left over from testing? If so, delete the imports as well.
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
File fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java:
http://gerrit.cloudera.org:8080/#/c/9697/19/fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java@47
PS19, Line 47: private final static Logger LOG = LoggerFactory.getLogger(ScalarFunction.class);
Either I'm confused about how this gets used, or I'm seeing a lot of extra Logger stuff lying around.
http://gerrit.cloudera.org:8080/#/c/9697/19/testdata/bin/copy-udfs-udas.sh
File testdata/bin/copy-udfs-udas.sh:
http://gerrit.cloudera.org:8080/#/c/9697/19/testdata/bin/copy-udfs-udas.sh@64
PS19, Line 64: > src/main/java/org/apache/impala/NewReplaceStringUdf.java
This is a pretty ugly hack; does this mean you can't run the test directly without also manually performing this step?
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 22:47:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 21: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 03:25:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#7).
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
IMPALA-6670: refresh lib-cache entries from plan (WIP)
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 317 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/7
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 7
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
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/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Reviewed-on: http://gerrit.cloudera.org:8080/9697
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 440 insertions(+), 83 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 23
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 21:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2167/
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 17:47:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java:
http://gerrit.cloudera.org:8080/#/c/9697/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@606
PS5, Line 606: fn_.UpdateLastModifiedTime();
> good point about the placement of this method. it seemed like a cleaner pla
Such a FE unit test could look something like this:
void testMtime(String expr, Long expectedMtime) {
Stmt stmt = AnalyzesOk("select " + expr);
Expr e = // get first select-list expr
TExpr texpr = e.toThrift()
// Check whether mtime is set (or not set), and has expected value
}
Write a test with a bunch of calls to testMtime() that cover the interesting cases like UDFs, built-ins etc,
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 15:57:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 22:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2169/
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 22
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Sat, 24 Mar 2018 00:49:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 21: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 21:26:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h@79
PS8, Line 79: /// exp_mtime differs from the mtime on the file system.
> indeed, I missed this one. this error comes up for two reasons: 1) the cach
So is it fair to say that if a non-OK status is returned, the cache will be purged of that entry? Seems worth a comment since I don't think the behavior is obvious.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@391
PS11, Line 391: protected TSymbolLookupParams getLookupParams() { return null; }
> that would make the class abstract, which I tried to avoid so I've replaced
Ahh good point. You are right. Let's definitely not to anything invasive here.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/main/java/org/apache/impala/catalog/Function.java@407
PS11, Line 407: // There was an error looking up the lib. A max mtime will result in an error.
> yeah, I don't like returning from here. I'd prefer to throw an exception an
I agree with your sentiment here. I think we should bubble up the exception, and have the caller wrap it in an IllegalStateException. We have a few existing places where toThrift() checks invariants before sending the BE that can possibly cause a crash, so at least there is some consistency even if it's not perfectly pleasing.
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2162
PS11, Line 2162: // HACK: aggregates contain slot-refs in their child expressions. they are
> I tried materializeRequiredSlots before but, for example, the avg has a cas
You can fix the hack with this (I tried it):
TupleDescriptor tblRefDesc = stmt.fromClause_.get(0).getDesc();
tblRefDesc.materializeSlots();
tblRefDesc.computeMemLayout();
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 11
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 22:44:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 21: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2164/
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 07:10:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/17/be/src/runtime/lib-cache.cc@202
PS17, Line 202: if (entry != nullptr) {
> According to the header comment, this function returns Status.ok() iff it s
updated the header to cover this case. the caller was setting a default value, but yes, it makes sense to set it here too.
note that entry can by null for builtins so dcheck would not work.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 17
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:24:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#12).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 384 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/12
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 12
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 5:
(2 comments)
Responding to comments, still studying the BE code
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9697/5/be/src/codegen/llvm-codegen.cc@801
PS5, Line 801: time_t mtime =
> its not set for builtins, which afaict, also use this code path.
Got it. Perhaps it's simpler and less bug prone to have a special value like -1 that is agreed upon by FE and BE to mean "built-in". That way we can avoid checking isset all over the place (if we forget one place, we could have a strange bug). The FE unit tests can make sure that this value is set.
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/5/tests/custom_cluster/test_coordinators.py@133
PS5, Line 133: db_name = 'TEST_EXEC_ONLY_CACHE'
> didn't work for custom cluster tests. I seem to get two tests instead of on
Hm, weird I'm not familiar with this issue. Happy to defer his cleanup, but the db dropping and client close should ideally be in a finally block
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 16:09:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h
File be/src/runtime/lib-cache.h:
http://gerrit.cloudera.org:8080/#/c/9697/8/be/src/runtime/lib-cache.h@79
PS8, Line 79: /// exp_mtime differs from the mtime on the file system.
> I think you might have missed this comment.
indeed, I missed this one. this error comes up for two reasons: 1) the cached entry needs a refresh or 2) we're adding a new entry.
For (1), a refresh is needed so the entry is out of date and removed as we've previously done. I think its better to conservatively clean-up/fail then hope other state catches up. The correction can be made when retrying. For (2), no state is modified-- now the cache is updated only after the entry loading is successful/correct.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 12
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 22:00:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 14:
(5 comments)
Happy with the change, only minor stuff left
http://gerrit.cloudera.org:8080/#/c/9697/14/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/9697/14/common/thrift/Frontend.thrift@644
PS14, Line 644: 8: required bool use_cache
Why not needs_refresh or force_refresh to be consistent with the refresh terminology in the backend? Arguably, any of the variants "use" the cache.
http://gerrit.cloudera.org:8080/#/c/9697/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:
http://gerrit.cloudera.org:8080/#/c/9697/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2168
PS14, Line 2168: // Checks that 'fnCall', when serialized to thrift, has mtime
Let's call 'fnCall' 'expr' for consistency. By 'fnCall' we typically mean FunctionCallExpr elsewhere in the code.
http://gerrit.cloudera.org:8080/#/c/9697/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@2178
PS14, Line 2178: TupleDescriptor int_desc = stmt.getAggInfo().intermediateTupleDesc_;
intDesc
http://gerrit.cloudera.org:8080/#/c/9697/14/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/14/tests/custom_cluster/test_coordinators.py@221
PS14, Line 221: check_call(["hadoop", "fs", "-put", "-f", new_src_path, tgt_path_2])
should we sleep a little before this to make sure we get a nte mtime? not sure what the mtime resolution is in hdfs
http://gerrit.cloudera.org:8080/#/c/9697/14/tests/custom_cluster/test_coordinators.py@228
PS14, Line 228: self.execute_query_expect_failure(client, mismatch_query)
* Also check the error message
* Run refresh functions (our recommended resolution) and check that the next time the query succeeds
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 14
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 04:08:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Zach Amsden, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#20).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 446 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/20
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 20
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 18
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 19:32:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/codegen/llvm-codegen-test.cc@107
PS9, Line 107: numeric_limits<long>::max()
how does that work now that the error check in libcache is equality of mtime?
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/exec/external-data-source-executor.cc
File be/src/exec/external-data-source-executor.cc:
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/exec/external-data-source-executor.cc@143
PS9, Line 143: numeric_limits<long>::max()
same question
http://gerrit.cloudera.org:8080/#/c/9697/9/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/9/fe/src/main/java/org/apache/impala/catalog/Function.java@407
PS9, Line 407: Long.MAX_VALUE;
same question about -- wont' this always end up erroring? is that intended?
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:00:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#15).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 445 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/15
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 15
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 17: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 17
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 17:18:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Zach Amsden, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#18).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 449 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/18
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 18
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/codegen/llvm-codegen-test.cc@107
PS9, Line 107: numeric_limits<long>::max()
> how does that work now that the error check in libcache is equality of mtim
yes, it should be -1 for the test. that means it won't be updated, but that's probably fine for the test.
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/exec/external-data-source-executor.cc
File be/src/exec/external-data-source-executor.cc:
http://gerrit.cloudera.org:8080/#/c/9697/9/be/src/exec/external-data-source-executor.cc@143
PS9, Line 143: numeric_limits<long>::max()
> same question
hmm, if its -1, ignore, we seem to get the regression (never update executors). otoh, external data sources only run on coordinators now (no parallelism), so this should never come up.
changed to -1 (ignore)
http://gerrit.cloudera.org:8080/#/c/9697/9/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/9/fe/src/main/java/org/apache/impala/catalog/Function.java@407
PS9, Line 407: Long.MAX_VALUE;
> same question about -- wont' this always end up erroring? is that intended?
yes, this will end up as an error. the suggestion will be to refresh and try again.
an alternative is to error here (in plan serialization at the coordinator).
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 16:28:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 19: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 19
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Mar 2018 20:06:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py
File tests/custom_cluster/test_coordinators.py:
http://gerrit.cloudera.org:8080/#/c/9697/8/tests/custom_cluster/test_coordinators.py@203
PS8, Line 203:
> thanks for trying, let me think whether I can come up with a sensible appro
I think I can get something when not all executors are used in a query... trying this now.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 11
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Mar 2018 18:51:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Zach Amsden, Dan Hecht, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#21).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 440 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/21
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 21
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
Patch Set 4:
latest update:
- fix several test cases found by core tests
- stricter check at executors to ensure that the same version of the file is used as the coordinator's version.
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 4
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 20 Mar 2018 02:30:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#14).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 433 insertions(+), 83 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/14
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 14
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan (WIP)
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#6).
Change subject: IMPALA-6670: refresh lib-cache entries from plan (WIP)
......................................................................
IMPALA-6670: refresh lib-cache entries from plan (WIP)
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 316 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/6
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 6
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9697
to look at the new patch set (#11).
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
IMPALA-6670: refresh lib-cache entries from plan
When an impalad is in executor-only mode, it receives no
catalog updates. As a result, lib-cache entries are never
refreshed. A consequence is that udf queries can return
incorrect results or may not run due to resolution issues.
Both cases are caused by the executor using a stale copy
of the lib file. For incorrect results, an old version of
the method may be used. Resolution issues can come up if
a method is added to a lib file.
The solution in this change is to capture the coordinator's
view of the lib file's last modified time when planning.
This last modified time is then shipped with the plan to
executors. Executors must then use both the lib file path
and the last modified time as a key for the lib-cache.
If the coordinator's last modified time is more recent than
the executor's lib-cache entry, then the entry is refreshed.
Brief discussion of alternatives:
- lib-cache always checks last modified time
+ easy/local change to lib-cache
- adds an fs lookup always. rejected for this reason
- keep the last modified time in the catalog
- bound on staleness is too loose. consider the case where
fn's f1, f2, f3 are created with last modified times of
t1, t2, t3. treat the fn's last modified time as a low-watermark;
if the cache entry has a more recent time, use it. Such a scheme
would allow the version at t2 to persist. An old fn may keep the
state from converging to the latest. This could end up with strange
cases where different versions of the lib are used across executors
for a single query.
In contrast, the change in this path relies on the statestore to
push versions forward at all coordinators, so will push all
versions at all caches forward as well.
Testing:
- added an e2e custom cluster test
Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
---
M be/src/codegen/codegen-callgraph.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/external-data-source-executor.cc
M be/src/exprs/agg-fn.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/runtime/lib-cache.cc
M be/src/runtime/lib-cache.h
M be/src/service/fe-support.cc
M common/thrift/Frontend.thrift
M common/thrift/Types.thrift
M common/thrift/generate_error_codes.py
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarFunction.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M testdata/bin/copy-udfs-udas.sh
M tests/custom_cluster/test_coordinators.py
M tests/test-hive-udfs/src/main/java/org/apache/impala/TestUpdateUdf.java
22 files changed, 379 insertions(+), 81 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/9697/11
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 11
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6670: refresh lib-cache entries from plan
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/9697 )
Change subject: IMPALA-6670: refresh lib-cache entries from plan
......................................................................
Patch Set 20:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9697/20/be/src/runtime/lib-cache.cc
File be/src/runtime/lib-cache.cc:
http://gerrit.cloudera.org:8080/#/c/9697/20/be/src/runtime/lib-cache.cc@410
PS20, Line 410: if (status.ok() && exp_mtime >= 0 && fs_last_modified_time != exp_mtime) {
> Would it be simpler to hoist this above line 404, then just set:
yes, good simplification. done.
http://gerrit.cloudera.org:8080/#/c/9697/20/fe/src/main/java/org/apache/impala/catalog/Function.java
File fe/src/main/java/org/apache/impala/catalog/Function.java:
http://gerrit.cloudera.org:8080/#/c/9697/20/fe/src/main/java/org/apache/impala/catalog/Function.java@57
PS20, Line 57: private final static Logger LOG = LoggerFactory.getLogger(Function.class);
> 1 more Logger here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/9697
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf740ea8c6a47e671427d30b4d139cb8507b7ff6
Gerrit-Change-Number: 9697
Gerrit-PatchSet: 20
Gerrit-Owner: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Mar 2018 02:37:25 +0000
Gerrit-HasComments: Yes