You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/08/15 06:51:54 UTC
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Hello Bharath Vissapragada, Tianyi Wang, Vuk Ercegovac,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/11231
to review the following change.
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
IMPALA-7447. Evict LocalCatalog cache entries based on size
This pulls in the 'sizeof' library from ehcache (Apache-licensed) and
uses it to implement size-based eviction of cache entries in
LocalCatalog.
This is difficult to test without being quite fragile to small changes
in the cached structures. However, I added a simple unit test as a
general sanity-check that it is computing some reasonable result.
Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
---
M be/src/runtime/exec-env.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
7 files changed, 109 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11231/1
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
File fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java:
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@111
PS3, Line 111: // the size loosely.
> nm the last point about sizes... saw that the other change (other tests in
There is a way to fake timing but I didn't want to duplicate the testing of the cache itself (ie I could advance time and make sure I get a miss on something where I previously got a hit, but doesnt seem too interesting). Is there a particular behavior on expiration that you're worried about?
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Aug 2018 23:19:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11231/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/11231/3/be/src/runtime/exec-env.cc@90
PS3, Line 90:
> nit: get rid of extra spaces (here and several words to the right)
Done
http://gerrit.cloudera.org:8080/#/c/11231/3/common/thrift/BackendGflags.thrift
File common/thrift/BackendGflags.thrift:
http://gerrit.cloudera.org:8080/#/c/11231/3/common/thrift/BackendGflags.thrift@87
PS3, Line 87: seconds
> for seconds, seems we just use "s" as a suffix. lets go with that for now f
Done
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 20 Aug 2018 23:14:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
File fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java:
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@111
PS3, Line 111: partialRefs);
> There is a way to fake timing but I didn't want to duplicate the testing of
its primarily to make sure that the flag we pass in for expiration is hooked up as expected. I'm fine with an end-to-end custom-cluster test covering that. perhaps add a todo for that?
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Aug 2018 21:13:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/445/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 02:20:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
File fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java:
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@111
PS3, Line 111: partialRefs);
> its primarily to make sure that the flag we pass in for expiration is hooke
k, I added a TODO at the cache instantiation site to add e2e tests to stress eviction
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 01:38:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11231
to look at the new patch set (#6).
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
IMPALA-7447. Evict LocalCatalog cache entries based on size
This pulls in the 'sizeof' library from ehcache (Apache-licensed) and
uses it to implement size-based eviction of cache entries in
LocalCatalog.
This is difficult to test without being quite fragile to small changes
in the cached structures. However, I added a simple unit test as a
general sanity-check that it is computing some reasonable result.
Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
---
M be/src/runtime/exec-env.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
7 files changed, 113 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11231/6
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/345/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Aug 2018 07:28:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Aug 2018 06:11:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 05:08:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/362/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 16 Aug 2018 00:26:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11231
to look at the new patch set (#4).
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
IMPALA-7447. Evict LocalCatalog cache entries based on size
This pulls in the 'sizeof' library from ehcache (Apache-licensed) and
uses it to implement size-based eviction of cache entries in
LocalCatalog.
This is difficult to test without being quite fragile to small changes
in the cached structures. However, I added a simple unit test as a
general sanity-check that it is computing some reasonable result.
Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
---
M be/src/runtime/exec-env.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
7 files changed, 111 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11231/4
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11231/3/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/11231/3/be/src/runtime/exec-env.cc@90
PS3, Line 90:
nit: get rid of extra spaces (here and several words to the right)
http://gerrit.cloudera.org:8080/#/c/11231/3/common/thrift/BackendGflags.thrift
File common/thrift/BackendGflags.thrift:
http://gerrit.cloudera.org:8080/#/c/11231/3/common/thrift/BackendGflags.thrift@87
PS3, Line 87: seconds
for seconds, seems we just use "s" as a suffix. lets go with that for now for consistency.
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
File fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java:
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@111
PS3, Line 111: // the size loosely.
for timing, does guava offer an interface to accept a fake clock to make time-based testing easier?
for size-based tests, if the number of entries can be accessed from the cache, perhaps dialing it down can test whether the cache contains expected number of entries.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sun, 19 Aug 2018 20:19:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6: Code-Review+2
carrying +2 from Vuk
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 16:05:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11231/6/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/11231/6/be/src/runtime/exec-env.cc@86
PS6, Line 86: DEFINE_int32_hidden(local_catalog_cache_mb, -1,
: "If --use_local_catalog is enabled, configures the size of the catalog "
: "cache within each impalad. If this is set to -1, the cache is auto-"
: "configured to 60% of the configured Java heap size. Note that the Java "
: "heap size is distinct from and typically smaller than the overall "
: "Impala memory limit.")
> IMO, from a end user POV, it is less confusing if this directly exposes the
oops, its hidden, nvm.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 23:32:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11231/6/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
http://gerrit.cloudera.org:8080/#/c/11231/6/be/src/runtime/exec-env.cc@86
PS6, Line 86: DEFINE_int32_hidden(local_catalog_cache_mb, -1,
: "If --use_local_catalog is enabled, configures the size of the catalog "
: "cache within each impalad. If this is set to -1, the cache is auto-"
: "configured to 60% of the configured Java heap size. Note that the Java "
: "heap size is distinct from and typically smaller than the overall "
: "Impala memory limit.")
IMO, from a end user POV, it is less confusing if this directly exposes the % configuration. Something like local_catalog_heap_max_percent...
Given we already have a max_heap (-Xmx), this would confuse users as to why they need to configure two separate values.
Anyway we probably don't expect the customers to change this often, so feel free to ignore if that is the case.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 23:27:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Tianyi Wang, Impala Public Jenkins, Vuk Ercegovac,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11231
to look at the new patch set (#2).
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
IMPALA-7447. Evict LocalCatalog cache entries based on size
This pulls in the 'sizeof' library from ehcache (Apache-licensed) and
uses it to implement size-based eviction of cache entries in
LocalCatalog.
This is difficult to test without being quite fragile to small changes
in the cached structures. However, I added a simple unit test as a
general sanity-check that it is computing some reasonable result.
Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
---
M be/src/runtime/exec-env.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
7 files changed, 109 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/11231/2
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3061/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 22 Aug 2018 01:47:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
File fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java:
http://gerrit.cloudera.org:8080/#/c/11231/3/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java@111
PS3, Line 111: // the size loosely.
> for timing, does guava offer an interface to accept a fake clock to make ti
nm the last point about sizes... saw that the other change (other tests in this file) address that.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sun, 19 Aug 2018 20:23:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/417/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Aug 2018 00:13:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
IMPALA-7447. Evict LocalCatalog cache entries based on size
This pulls in the 'sizeof' library from ehcache (Apache-licensed) and
uses it to implement size-based eviction of cache entries in
LocalCatalog.
This is difficult to test without being quite fragile to small changes
in the cached structures. However, I added a simple unit test as a
general sanity-check that it is computing some reasonable result.
Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Reviewed-on: http://gerrit.cloudera.org:8080/11231
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M be/src/runtime/exec-env.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M fe/pom.xml
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java
M fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
7 files changed, 113 insertions(+), 12 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 7
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-7447. Evict LocalCatalog cache entries based on size
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11231 )
Change subject: IMPALA-7447. Evict LocalCatalog cache entries based on size
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3049/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/11231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia96af49b35c17e505b7b6785e78d140939085d91
Gerrit-Change-Number: 11231
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 21 Aug 2018 02:42:25 +0000
Gerrit-HasComments: No