You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tianyi Wang (Code Review)" <ge...@cloudera.org> on 2018/08/15 18:50:52 UTC
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Tianyi Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11224
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
This feature can be enabled with an catalogd CLI arg
--unused_table_ttl_sec. Periodically the HDFS tables that are not used
in unused_table_ttl_sec is invalidated from catalogd. The table usage
is reported by impalad to catalogd when a query/DML finishes its
execution. This patch also enables manual eviction from catalog webpage
/invalidate_lru_table for experiment purposes.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-server.h
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/client-request-state.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A tests/custom_cluster/test_automatic_invalidation.py
21 files changed, 332 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/1
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72
PS3, Line 72: the
: /// numbers of their usages
> disagree - frequency implies some division, doesn't it?
per time unit.. I thought it was ok to use this for a time period in general, which in this case, is the time since the last report?
Something about "number of their usages" seems off though, perhaps just "use count"?
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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, 27 Aug 2018 20:19:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 2:
(46 comments)
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@537
PS1, Line 537:
> How do you measure the length of a string literal without using an array?
I mean:
static const char* kMsg = "unused_table_ttl_sec not specified";
Value value(kMsg, strlen(kMsg);
Even though strlen looks like a function call here, gcc -O1 or higher will optimize out the strlen to just the known constant size, see https://godbolt.org/z/3AMhwL
http://gerrit.cloudera.org:8080/#/c/11224/2/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/2/be/src/common/global-flags.cc@208
PS2, Line 208: Talbe
typo: Table
nit: I think it's better to avoid talking too much about implementation mechanism in the doc here. How about something like:
If a table has not been referenced in a query for more than the configured amount of time, the catalog server will automatically evict its cached metadata about this table. This has the same effect as a user-initiated "INVALIDATE METADATA" statement on the unused table. Configuring this to 0 disables automatic invalidation of tables.
http://gerrit.cloudera.org:8080/#/c/11224/2/be/src/common/global-flags.cc@207
PS2, Line 207: DEFINE_int32(unused_table_ttl_sec, 0, "Catalogd invalidates unused tables older than this"
: "threshold. Talbe usage is reported periodically from impalad to catalogd. Then"
: "catalog invalidates old unused tables as if they are invalidated by a user."
: "0 disables this feature.");
:
: DEFINE_bool(invalidate_tables_on_memory_pressure, false, "Catalog invalidates recently"
: "unused tables when the old GC generation is almost full.");
nit: missing extra spaces at end of each line before the end quote
http://gerrit.cloudera.org:8080/#/c/11224/2/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/2/be/src/exec/catalog-op-executor.h@72
PS2, Line 72: Status ReportTableUsage(const TReportTableUsageRequest& req,
nit: add some docs
http://gerrit.cloudera.org:8080/#/c/11224/2/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:
http://gerrit.cloudera.org:8080/#/c/11224/2/common/thrift/CatalogService.thrift@415
PS2, Line 415: num_usage
nit: I think "usage_count" or "num_usages" would be better
http://gerrit.cloudera.org:8080/#/c/11224/2/common/thrift/CatalogService.thrift@419
PS2, Line 419: 1: required map<CatalogObjects.TTableName, TTableUsage> tables;
nit: extra space in here
http://gerrit.cloudera.org:8080/#/c/11224/2/common/thrift/CatalogService.thrift@423
PS2, Line 423:
nit: no need for empty line
http://gerrit.cloudera.org:8080/#/c/11224/2/common/thrift/CatalogService.thrift@461
PS2, Line 461: TReportTableUsageResponse ReportTableUsage(1: TReportTableUsageRequest req);
nit: doc
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@141
PS2, Line 141: if (ImpaladTableUsageTracker.INSTANCE != null) {
: ImpaladTableUsageTracker.INSTANCE.addTables(tbls);
: }
I think we need to do this both on the views _and_ the expanded tables, not just the top-level views. Otherwise, if some tables are only accessed via views, we'll never update their lastUsed time, and they'll get constantly evicted.
If we do this at the end of the function instead of at the top, then I think we'll cover both the views and the underlying tables, right?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@255
PS1, Line 255: catalogdTableShrinker_ = new CatalogdTableShrinker(this,
> It's initialization scope like a constructor
hm, maybe it would be simpler to just put this as a local variable inside 'run'?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@252
PS2, Line 252: throw new IllegalArgumentException("unused_table_ttl_sec flag must be a " +
: "non-negative integer.");
I think doing this validation in the ctor below makes more sense -- maybe just plumb the gflags structure in directly so that you can keep all of the validation with the code that uses it, and use a factory method so you can return null if it's disabled in the configuration?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1920
PS2, Line 1920: for (TTableName tableName : req.tables.keySet()) {
nit: indentation
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1935
PS2, Line 1935: setCatalogdTableShrinker
is this for tests? If so please annotate @VisibleForTesting
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java:
PS2:
license
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@23
PS2, Line 23: table
nit: tables
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@30
PS2, Line 30: CatalogdTableShrinker
I think "Invalidator" is better than "Shrinker", because we are actually evicting whole tables, not shrinking them down to some compressed representation.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@32
PS2, Line 32: // Plugable time source for tests. Defined as static to avoid passing
nit: use /** javadoc format */ so that IDEs can highlight the docs nicely
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@34
PS2, Line 34: static Ticker timeSource_ = Ticker.systemTicker();
nit: probably should be TIME_SOURCE as something that's mostly-constant. You can annotate @VisibleForTesting to make it clearer why it exists.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@35
PS2, Line 35: private CatalogServiceCatalog catalog_;
can this be final?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@55
PS2, Line 55: ne.addNotificationListener(new NotificationListener() {
I think breaking out the NotificationListener subclass and Thread subclass below into their own private inner classes would make this file a bit more readable
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@63
PS2, Line 63: if (!info.getGcAction().equals("end of major GC")) return;
oof, is this the only way to do this? what other kind of events might we see? looking at the fields of 'getMemoryUsageAfterGc', it seems like all events are "GC finished".
One thought: I know I'd suggested using the notification listener interface initially, but if we already have a thread that is doing periodic checks for TTL purposes, maybe it's easier to eliminate a bunch of this code and instead just use GarbageCollectorMXBean.getLastGcInfo() in that notification loop? If, in that loop, we see that the GC count for old gen increased, and the usage after the GC was still high, we can do the cleanup? then we don't have the complexity of the 'gcTrigger' wait/notify
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@65
PS2, Line 65: MemoryUsage tenuredGenUsage = mem.get("PS Old Gen");
is this always the name of the old gen, even if a different collector is configured? Could this be null if enabling some future GC like Shenandoah, etc? I wonder if there is any less fragile way to find the old gen
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@66
PS2, Line 66: 7 / 10
please add a constant for this fraction. I think it's fine to use floating point math here to make things simpler.
Also, I think we should consider a slightly lower amount, even though it might seem wasteful. My reasoning is that the default configuration for CMSInitiatingOccupancyFraction is 68%. That's the percent heap usage at which a concurrent GC is triggered. So, if, *after* a GC has finished, we have more than 70% heap usage, then we'll have already triggered a back-to-back second CMS cycle. If instead we triggered this at 60% post-GC, then we'd have time to run our eviction logic before reaching the 68% threshold again, and we'd actually be able to collect heap in the next triggered GC.
Making this configurable, if it's not too much of a pain in the ass, might be useful for us to test and determine a good default empirically.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@89
PS2, Line 89: .timedWait(CatalogdTableShrinker.this, unusedTableTtlSec_);
this doesn't seem quite right. Even if the TTL is 100 seconds, that doesnt' mean we only want to _check_ every 100 seconds. Otherwise, we could see a table when it was used 99sec ago, and then go back to sleep for another 100 seconds
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@117
PS2, Line 117: invalidate10percent
think this should be coded more generically to invalidate a provided ratio, and have a config for that ratio
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@124
PS2, Line 124: // TODO: use quick select
as an alternative, why not use a PriorityQueue above instead of a List? You can determine the 10% count by doing one pass through the tables/dbs, and then limit the size of the priority queue to the computed count. Of course the count could change slightly in between but it probably doesn't matter.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@135
PS2, Line 135: catalog_.invalidateTable(tTableName, tblWasRemoved, dbWasAdded);
let's LOG.info here
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@144
PS2, Line 144: retireAgeSecs * 1000000000L
I think using one of the TimeUnit conversion functions is easier to read here
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@148
PS2, Line 148: catalog_.invalidateTable(tTableName, tblWasRemoved, dbWasAdded);
same
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/Db.java@26
PS2, Line 26: import org.apache.impala.thrift.TReportTableUsageRequest;
unused
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@114
PS2, Line 114: throw new IllegalArgumentException("unused_table_ttl_sec flag must be a " +
same comment as elsewhere - better to keep the validation with the code that uses these configs
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@117
PS2, Line 117: ImpaladTableUsageTracker.create();
shouldn't you be assigning this to the member variable?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@32
PS2, Line 32: /**
: * Report table usage to catalogd.
: */
can you expand this javadoc to talk a bit more about the role of this class?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@36
PS2, Line 36: ImpaladCatalog
nit: wrong class name
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@43
PS2, Line 43: reportThread
can you give this thread a name?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@54
PS2, Line 54: INSTANCE = new ImpaladTableUsageTracker();
can we avoid having yet another singleton? Singletons make testing harder, etc. Maybe this could be instantiated as a member of Frontend? Seems like the table metadata loader could grab it from Frontend pretty easily.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@61
PS2, Line 61: tableName :
style nit: we seem to more commonly skip the space in between the variable name and the ':'. I don't really care but Vuk pointed this out in a review earlier today so figured I'd note it :)
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@73
PS2, Line 73: while (true) {
I think it would be good to add a sleep in here as well so that we rate limit the reporting of usage to once every few seconds or something. There isn't any need to report usage immediately, and this will reduce the load on catalogd, etc. If that makes testing inconvenient, you could put in a @VisibleForTesting global that you could set back down to 0 to encourage tests to run faster.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@83
PS2, Line 83: LOG.warn("Thrift serialization failure: ", e);
: } catch (Exception e) {
: LOG.error("Unexpected exception thrown: ", e);
I think you could probably combine both of these into a multi-catch close, and probably should downgrade the ERROR log to WARN since it is a recoverable error (no incorrect results or failed queries resulted). The error message should also be clearer to the user what the effect of the error was. For example, "Unable to report table usage information to catalog server"
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@89
PS2, Line 89:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
PS2:
do we need to mark usage for all of the other calls in this file?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@728
PS2, Line 728: refreshLastUsedTime
why not just put this in 'load()' since it seems you always call it after load?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableShrinkerTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogdTableShrinkerTest.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/test/java/org/apache/impala/catalog/CatalogdTableShrinkerTest.java@92
PS2, Line 92: synchronized (cleaner) {
: cleaner.notify();
: }
this seems to be coupled pretty tightly to the implementation details of the class under test. Can we add some function like 'wakeNowForTests()' instead?
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py@42
PS2, Line 42: time.sleep(20)
if we add a log message when auto-invalidation happens, we can change this to tail the logs and wait for it, right? then we'd be less sensitive to test timing
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py@51
PS2, Line 51: """IMPALA-5789: Test that always false filter filters out splits when file-level
: filtering is disabled."""
? same above
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py@60
PS2, Line 60: time.sleep(20)
: assert metadata_cache_string not in urllib.urlopen(url).read()
:
similarly, to avoid test timing dependency, would be better to loop here and check until it succeeds (with a long timeout)
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Tue, 21 Aug 2018 01:42:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 22:54:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
(11 comments)
still reviewing, but wanted to send out some comments from my end. main question is about the complexity to use/tune the feature.
http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG@12
PS3, Line 12: unused_table_ttl_sec
use a common prefix for these flags so that the user knows they're related. perhaps "invalidate_tables_timeout_s"?
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@211
PS3, Line 211: "this to 0 disables automatic invalidation of tables.");
based on ttl or in general? the way I read this is that ttl and memory pressure can both be "on". the memory pressure one, if its on, can be further refined by the catalogd_table_invalidator* flags.
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@214
PS3, Line 214: "invalidate recently unused tables when the old GC generation is almost full.");
put a reference here to the two catalogd_table flags to further control behavior?
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@216
PS3, Line 216: DEFINE_double(catalogd_table_invalidator_old_gen_full_threshold, 0.6, "The ratio above "
This is a lot of new flags. Perhaps we just expose the memory_pressure one and the ttl one and make the memory related ones hidden? I wouldn't know off-hand how to tune this and would most likely just turn off the memory-based one.
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72
PS3, Line 72: servier
nit: server
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72
PS3, Line 72: the
: /// numbers of their usages
nit: their usage frequency
http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:
http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift@419
PS3, Line 419: TTableUsage
any reason to not simplify this to just an i32?
http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift@460
PS3, Line 460: the numbers of their usages
nit: their usage frequency
http://gerrit.cloudera.org:8080/#/c/11224/3/common/thrift/CatalogService.thrift@461
PS3, Line 461: Report
nit: Report is unclear who is sending whom information (is the caller getting a report or updating something?). Perhaps "UpdateTableUsage"?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@74
PS3, Line 74: maybeAddTables
nit: maybeAddTables is a bit unclear. perhaps, recordTableUsage?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/TableLoader.java
File fe/src/main/java/org/apache/impala/catalog/TableLoader.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@81
PS3, Line 81: table.refreshLastUsedTime();
I forget what validate does, but if it can throw an exception, perhaps update the use after the validate?
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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, 27 Aug 2018 19:23:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
Stats extrapolation is an example of enabling a feature via a table property:
https://github.com/apache/impala/blob/2.x/tests/metadata/test_stats_extrapolation.py
https://github.com/apache/impala/blob/2.x/testdata/workloads/functional-query/queries/QueryTest/stats-extrapolation.test
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Fri, 31 Aug 2018 18:11:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/598/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 06 Sep 2018 02:31:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 7:
(8 comments)
Just a few small notes remaining from me.
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@175
PS4, Line 175: icted because o
> The call site of this function is guarded with a try clause so I think it's
I don't think relying on catching NullPointerException is the right approach, though -- that would be very concerning for an operator to see "Unexpected exception: NullPointerException" in the logs.
Under what circumstance does it return null? Perhaps we should return false with a warning if it does?
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@75
PS7, Line 75: AtomicLong triggerCount_ = new AtomicLong();
probably worth a javadoc on this because it wasn't entirely clear to me what it meant. When I first saw it, I thought it was the number of times a table had been invalidated due to this class, but actually it looks like it's just the number of times the background thread woke up and did a scan. Maybe 'scanCount_' would be better?
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@150
PS7, Line 150: if (!(gcbean instanceof NotificationListener)) {
I think this is now misplaced -- shouldn't this be inside the loop after the check for 'Old'? (but above the setting of member vars, as Vuk caught)
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@225
PS7, Line 225: "Table " + table.getFullName() + " is invalidated
nit: I think "Invalidated table foo.bar" is clearer than "foo.bar is invalidated" (or as you had it before with no "is")
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@273
PS7, Line 273: ly" +
nit: missing space
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@274
PS7, Line 274: w
nit: capitalize.
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java:
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@55
PS7, Line 55: atalog_
: .invalidateTable(new TTableName(dbName, tblName),
: tblWasRemoved, dbWasAdded);
nit: can now reformat this code in a less strange way
http://gerrit.cloudera.org:8080/#/c/11224/7/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/7/tests/custom_cluster/test_automatic_invalidation.py@45
PS7, Line 45: if "Table functional.alltypes is invalidated due to inactivity" in \
We have self.assert_impalad_log_contains which does this in a bit nicer way. Also, I think this loop should eventually give up rather than causing a test timeout if it never shows up. Check out test_restart_catalogd in test_local_catalog.py for an example.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Fri, 07 Sep 2018 01:35:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 11: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 22:37:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72
PS3, Line 72: the
: /// numbers of their usages
> per time unit.. I thought it was ok to use this for a time period in genera
yea. 'use count' sounds fine, or 'access_count'
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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, 27 Aug 2018 20:23:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py@62
PS2, Line 62:
flake8: W391 blank line at end of file
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Tue, 21 Aug 2018 00:29:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/470/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Fri, 24 Aug 2018 03:34:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11224/3/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/3/tests/custom_cluster/test_automatic_invalidation.py@64
PS3, Line 64:
flake8: W391 blank line at end of file
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Fri, 24 Aug 2018 03:00:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/634/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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, 10 Sep 2018 21:46:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 1:
(30 comments)
http://gerrit.cloudera.org:8080/#/c/11224/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11224/1//COMMIT_MSG@11
PS1, Line 11: HDFS tables
why only HDFS tables?
http://gerrit.cloudera.org:8080/#/c/11224/1//COMMIT_MSG@13
PS1, Line 13: query/DML finishes its
: execution
curious why at end of execution instead of at end of planning?
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@165
PS1, Line 165: UpdateUsedTableNames
nit: i think "ReportTableUsage" might be a better name? This current name makes it sound like it is replacing a list instead of just marking that the tables got used. And, maybe we might want to report some more details such as the count of queries in a particular interval that used the table.
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@169
PS1, Line 169: if (!status.ok()) LOG(ERROR) << status.GetDetail();
this should probably be WARNING instead of ERROR since it doesn't affect correctness.
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@171
PS1, Line 171: void InvalidateUnusedTables(TInvalidateUnusedTablesResponse& resp,
any particular reason to expose this via RPC instead of just automatic in the background?
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@537
PS1, Line 537: const char msg[] = "unused_table_ttl_sec not specified";
I think if you just pass a string constant, it'll end up in static storage, and you wouldn't need to use the "copying" approach right?
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@539
PS1, Line 539: document->AddMember("error", value, document->GetAllocator());
maybe just early-return here so that the rest of the function doesn't need to be nested?
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@542
PS1, Line 542: unused_table_ttl_sec = boost::lexical_cast<int>(unused_table_ttl_sec_it->second);
can you tighten the "catch" block to only cover this line, and then early-return from it? That way the majority of the function won't be indented.
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@547
PS1, Line 547: if (status.ok()) {
if not OK don't we want to put this in the 'error'? I think if we can refactor this whole function into another one that just returns Status, then we could consolidate all of the various code that fills in the "error" field in the document into one place
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@549
PS1, Line 549: tables
nit: can we name this thrift field "removed_tables" or "invalidated_tables" or something so it's clearer?
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@570
PS1, Line 570: const char msg[] = "invalid unused_table_ttl_sec value";
: Value value(msg, arraysize(msg));
:
nit: indentation off
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/common/global-flags.cc@202
PS1, Line 202: catalog invalidates tables older than this"
: "threshold
nit: I think this doc should be expanded out to explain a bit more about the usage/semantics.
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/exec/catalog-op-executor.h@76
PS1, Line 76: TInvalidateUnusedTablesResponse* resp);
nit: indentation
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/exec/catalog-op-executor.cc
File be/src/exec/catalog-op-executor.cc:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/exec/catalog-op-executor.cc@336
PS1, Line 336: InvalidateUnusedTable
nit: this method name is missing an 's'
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/service/client-request-state.cc@528
PS1, Line 528: // Update used tables in this query.
I think this should be deferred onto some queue rather than done inline, otherwise this can block the user, right? eg if we have a bounded queue of usages, and some background thread which pulls items off the queue once every few seconds and sends them to catalogd, we avoid this critical-path RPC
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/service/client-request-state.cc@534
PS1, Line 534: req.tables.emplace_back(name);
nit: std::move(name)
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/service/client-request-state.cc@537
PS1, Line 537: catalog_op_executor_.reset(
: new CatalogOpExecutor(exec_env_, frontend_, server_profile_));
hrm, I'm surprised this is a member variable instead of just in local scope. If it's a member variable, is it already initialized here?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@84
PS1, Line 84: import static jodd.util.ThreadUtil.sleep;
I think this is coming by some accidental transitive dependency. Why not just use Thread.sleep or Guava's Uninterruptibles.sleep if you don't want to handle interruption?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@252
PS1, Line 252: if (unusedTableTtlSec != 0) {
can we do a check that it's not negative?
also, mind refactoring this code into a new method like 'startAutoInvalidationThread' or something?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@253
PS1, Line 253: new Thread(new Runnable() {
this thread should have a name and also probably be marked as a daemon thread.
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@255
PS1, Line 255: {
what's with this scope?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@262
PS1, Line 262: invalidateUnusedTables(req);
I think it makes sense to put a try/catch around this so that, even if the call throws some kind of weird runtime exception, we can log it and keep going. Otherwise we'll silently lose this thread and start to "leak" memory
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1916
PS1, Line 1916: getTable(tableName.db_name, tableName.table_name).
do we have to worry about null here? seems plausible that a table might have gotten renamed/removed
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1925
PS1, Line 1925: Calendar oldestTableAllowed = null;
I think it would be safer to avoid any calendar calculations and instead just use a monotonic timestamp (System.nanotime gives you a monotonic clock)
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1935
PS1, Line 1935: FeFsTable
why only evict FeFsTable? I agree they are the largest, but seems unexpected to ignore the other tables. If we are going to do this, I think it makes sense to actually put the 'lastUsedTime' bit in the HdfsTable class instead of in the Table class, since that will also avoid the extra memory usage in 'IncompleteTable'.
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1935
PS1, Line 1935: req.isSetUnused_table_ttl_sec()
isn't this handled up above?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1936
PS1, Line 1936: table.lastUsedTime
can we add a getter?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/Table.java@111
PS1, Line 111: protected Calendar lastUsedTime;
this seems bulky. Per comment elsewhere how about just using a long, and setting it to System.nanotime?
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java:
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@29
PS1, Line 29: import com.google.common.collect.Iterables;
unused
http://gerrit.cloudera.org:8080/#/c/11224/1/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/1/tests/custom_cluster/test_automatic_invalidation.py@24
PS1, Line 24: class TestAutomaticCatalogInvalidation(CustomClusterTestSuite):
I think this test would probably be easier to write in Java so you can directly inspect the state of the Catalog instead of having to do it e2e like this.
If you do this in the Java side, you could also easily mock the advancement of time. For example, you could add a Guava 'Ticker' as a static member of CatalogServiceCatalog. In normal runtime the ticker would be set to be the systemTicker, and in the test, you can mock it out to make time advance deterministically.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@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-Comment-Date: Wed, 15 Aug 2018 19:55:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@225
PS4, Line 225:
> We don't know how much memory is released until a GC.
ok, I was looking at our own memory estimate of tables (it gets computed when serializing to thrift-- that will not happen with v2) as a proxy for how much we expect to reclaim. no need to change this for this round.
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java:
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@148
PS7, Line 148: fe_.getImpaladTableUsageTracker().recordTableUsage(loadedTbls_.keySet());
nit: formatting
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@175
PS4, Line 175: icted because o
> The call site of this function is guarded with a try clause so I think it's
if we know it can be hit, is the main reason not to guard it for readability?
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java:
http://gerrit.cloudera.org:8080/#/c/11224/7/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@76
PS7, Line 76: * invalidation is disabled.
nit: , but in that case, it will be a no-op.
http://gerrit.cloudera.org:8080/#/c/11224/7/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/7/tests/custom_cluster/test_automatic_invalidation.py@35
PS7, Line 35: query = "select count(*) from functional.alltypes"
add a brief description of what each test is trying to do. I expected that the same test is applied to v1 and v2 but was surprised to see two fairly different approaches when validating the state of catalogd (which is the same for both)-- why the difference?
http://gerrit.cloudera.org:8080/#/c/11224/7/tests/custom_cluster/test_automatic_invalidation.py@57
PS7, Line 57: mns (list) = list<struc
are you using this just for some evidence that its not an incomplete table?
http://gerrit.cloudera.org:8080/#/c/11224/7/tests/custom_cluster/test_automatic_invalidation.py@62
PS7, Line 62: urllib.urlope
perhaps use the methods in tests/common/impala_service.py, such as read_debug_webpage? they also include some looping with timeouts-- the use here might lead to flakes.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Fri, 07 Sep 2018 05:45:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
(42 comments)
http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG@13
PS3, Line 13: 2. If the old GC generation is almost full, a centen percentage of LRU
certain
http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG@14
PS3, Line 14: could be enabled
nit: "can be enabled" (if you say "could" it makes me think that this patch chose not to implement it but maybe will add that in the future, but it seems this is in the patch)
(I don't really care that much in the context of the commit message, just wanted to give the tip for better writing clarity)
http://gerrit.cloudera.org:8080/#/c/11224/3//COMMIT_MSG@18
PS3, Line 18: unsed
nit: used
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/catalog/catalog-util.cc@256
PS3, Line 256: bool TTableName::operator<(const impala::TTableName& that) const {
hmm... it seems risky to do operator-overloading on 'operator<' but not operator>, operator>=, operator==, etc. The style guide (https://google.github.io/styleguide/cppguide.html#Operator_Overloading) also says: "If you define an operator, also define any related operators that make sense, and make sure they are defined consistently. For example, if you overload <, overload all the comparison operators, and make sure < and > never return true for the same arguments."
Given that, I think it's better to define this as a named function/functor TableNameLessThan and specify that where necessary
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/catalog/catalog.h
File be/src/catalog/catalog.h:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/catalog/catalog.h@121
PS3, Line 121: Status ReportTableUsage(const TReportTableUsageRequest& req);
nit: doc
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/common/global-flags.cc@220
PS3, Line 220: ratio of "
: "the numbers
I think it's clearer to say "the fraction of tables" and also name the flag with the word "fraction" instead of "ratio". I think this is more consistent terminology with other software -- eg there are various JVM flags ending in "Ratio" and they take integers indicating a "1:N" ratio rather than a floating point fraction.
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/service/fe-support.cc@541
PS3, Line 541: status.AddDetail("Error making an RPC call to Catalog server.");
shouldn't this 'status' go somewhere? eg use it to throw an exception or put it into 'result.status' or something? As is, it's just ignored.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
File fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java@150
PS3, Line 150: ImpaladTableUsageTracker.maybeAddTables(fe_.getImpaladTableUsageTracker()
I guess you are doing this so that you can handle a 'null' usage tracker, but seems like it's not worth the confusion. Wouldn't it be simpler to just always instantiate the usage tracker, and have it no-op out the calls if disabled? (or even create a simple interface and instantiate a "noop" implementation of that interface, so you dont have to worry about a null)
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2002
PS3, Line 2002: ReportTableUsage
style nit: lower case 'r'
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@69
PS3, Line 69: * invalidation is executed.
nit: was executed
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@85
PS3, Line 85: if (invalidateTableOnMemoryPressure) {
do you mind refactoring out the contents of this block to a new function like "bool tryInstallGcListener()" or something, just to keep the constructor a bit more tidy?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@98
PS3, Line 98: ((NotificationEmitter) gcbean)
is this cast guaranteed safe? I think it's probably worth an instanceof check before downcast just to insulate us from a potential JVM change down the line (eg if they decide to stop supporting notifications, we will gracefully degrade)
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@105
PS3, Line 105: in the GC.
"in the GC MXBean"
nit: as a general rule, we should try to have warning messages explain what the effect is. In this case, we should probably say something like "Continuing without GC-triggered invalidation of tables." or "Continuing with only TTL-based table invalidation" or something like that, so that the operator knows that there was a problem, but that its effect is limited to that particular functionality.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@112
PS3, Line 112: daemon thread
nit: I think the "daemon thread" part is self-explanatory, since the jstack output already indicates whether or not a thread is a daemon
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@121
PS3, Line 121: if (unusedTableTtlSec < 0) {
: throw new IllegalArgumentException(
: "unused_table_ttl_sec flag must be a " + "non-negative integer.");
this can just be a Preconditions.checkArgument() with the same effect
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@138
PS3, Line 138: private boolean detectGC() {
This function should be renamed to indicate its full scope. When I first read the name I thought it was only detecting whether a GC occurred, but in fact it is also checking whether the post-GC heap was too full. Maybe a better name would be something like "shouldEvictFromFullHeapAfterGC", and add a short javadoc to indicate?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@154
PS3, Line 154: if (table.isLoaded()) tables.add(table);
can we add some kind of check here that the table's lastUsedTime is non-zero? I'm a little afraid of us introducing a bug or race where a table gets loaded but doesn't get its lastUsedTime set, and then we end up evicting it very quickly after. That would be a tricky bug for us to diagnose.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@178
PS3, Line 178: long inactivityTime = now - table.getLastUsedTime();
same as above -- we should be careful about not exposing a "zero" lastUsedTime. Perhaps we could even just put this check in 'getLastUsedTime' itself as a Preconditions.checkState?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@186
PS3, Line 186: seconds
nit: space before 'seconds'
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@89
PS2, Line 89:
> If we want the timeout to be accurate we need to keep a list of timeout in
agreed that absolute accuracy would require the full list of expiration times in a priority queue or something. However, right now the error is up to 2x of the configuration. If we instead always slept for 1 second between checks, then we would only be up to 1 second "late". I think a reasonable amount of error might be to sleep for min(5 minutes, 10% of the configured interval) or something -- this ensures that no matter how long your expiration interval is, we'd be at most 5 minutes late. And if you set a short interval like 60 seconds for testing purposes, we'll only be 6 seconds late.
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java@124
PS2, Line 124:
> I think that's still O(n lgn) and only marginally better than sorting.
I was thinking something like:
int count = 0;
for (Db db : catalog_.getAllDbs()) {
for (Table t : db.getTables()) {
if (t.isLoaded()) count++;
}
}
int numToRemove = count/10;
PriorityQueue<Table> toRemove = new PriorityQueue<>(...comparator...);
for (Db db : catalog_.getAllDbs()) {
for (Table t : db.getTables()) {
toRemove.add(t);
if (toRemove.size() > numToRemove) toRemove.poll();
}
}
then we end up with O(k) space and O(n lg k) time.
I guess your point is valid in that k = n/10 here so the runtimes are asymptotically equivalent. So, maybe the savings here aren't that big a deal. Up to you.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/Db.java@26
PS3, Line 26: import org.apache.impala.thrift.TReportTableUsageRequest;
unused? also, sorting
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@26
PS3, Line 26: import org.apache.hadoop.yarn.webapp.hamlet2.Hamlet;
oops?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@101
PS3, Line 101: private ImpaladTableUsageTracker impaladTableUsageTracker_;
unused?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@34
PS3, Line 34: and the data
think the word "report" is missing in here
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@39
PS3, Line 39: @VisibleForTesting
this isn't visible
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@61
PS3, Line 61: if (unusedTableTtlSec < 0) {
: throw new IllegalArgumentException(
Preconditions.checkArgument
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@83
PS3, Line 83: 0
1?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@93
PS3, Line 93: Thread.sleep(REPORT_INTERVAL_MS);
I think it's worth adding some randomness here so that, in a large cluster, the reports from multiple nodes don't all get synchronized. Keep in mind that typically all impalads start at the same time, and also that some blocking event on the catalogd side (like a GC pause) will end up encouraging periodic requestors to all be on the same synchronized period. If we randomly sleep for between 0.5 and 1.5x the report interval then the requests will nicely spread out and not cause any spikes on the remote node.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@96
PS3, Line 96: while (req_.tables.isEmpty()) wait();
it may be better to just 'continue' if it's empty. Otherwise, in the normal case of an idle Impalad which gets its first query (which has multiple tables referenced), we'll end up waking on the first table reference, immediately constructing a report for that first table, and then going back to sleep. So, the query causes two reports instead of just one. If instead we just went back to sleep here we'd probably only send one report.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/Table.java
File fe/src/main/java/org/apache/impala/catalog/Table.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/Table.java@108
PS3, Line 108: nanoseconds
should probably document "as returned by CatalogdTableInvalidator.nanoTime". Also should probably document that this is only set on the catalogd side, not kept up to date on the impalad, right?
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/main/java/org/apache/impala/catalog/Table.java@173
PS3, Line 173: public long getLastUsedTime() { return lastUsedTime_; }
can we Preconditions.checkState(!storedInImpaladCatalogCache_) here? this should only be called on the catalog side I think?
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/11224/2/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@728
PS2, Line 728:
> There is a load() of each type of table ...
what about putting it in 'getExistingTable'? that seems to be called any time a table is touched.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java:
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@47
PS3, Line 47: "functional", "alltypes"),
dbName, tblName
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@50
PS3, Line 50: CatalogdTableInvalidator.TIME_SOURCE = ticker;
probably needs an equivalent "undo" of this back to System ticker or else other tests that run after this one in the same JVM will continue to be polluted by this test setup.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@52
PS3, Line 52: 2, false, 0.6, 0.1)
can you add comments like: /*paramName=*/2, /*otherParamName=*/false here? These can be checked for validity using ErrorProne (https://github.com/google/error-prone/blob/master/docs/bugpattern/argumentselectiondefects/NamedParameters.md) and provide some nice documentation value at the call site. I normally try to use this style of comments whenever there's a function that takes several arguments of the same or similar type since it's easy to accidentally swap them.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@53
PS3, Line 53: assert !catalog_.getDb(dbName).getTable(tblName).isLoaded();
for Java unit tests we should use Assert.assertTrue and Assert.assertFalse. I'm not sure if we enable Java assertions ('-ea' flag on the JVM) for our test runs at the moment.
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@56
PS3, Line 56: assert table.getLastUsedTime() == 0;
this assertion surprises me. Shouldn't a table, when loaded, have a "lastUsedTime" of now? Is this just because the ticker is starting at time 0? Maybe it would be clearer if the ticker started at time 1000 or something, so ti's clear that we're asserting that the initial "lastUsedTime" is the time that the table was loaded
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@58
PS3, Line 58: // Get notified when the shrinking is done and proceed with the tests.
instead of the callback and this clever Boolean[] thing, what if we just added a @VisibleForTesting integer inside CatalogdTableInvalidator called 'triggerCount' that we could loop on and check once every 10ms or something? I think net lines of code would be far less and easier to follow. Saving the extra few milliseconds by using wait/notify isn't worth the code complexity in the context of a test
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@69
PS3, Line 69: nanoSecsPerSec
nit: make this a class-level constant NANOS_PER_SEC, or just use TimeUnit APIs to convert to/from
http://gerrit.cloudera.org:8080/#/c/11224/3/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@81
PS3, Line 81: catalog_.setCatalogdTableInvalidator(null);
the stopping and invalidation should probably be an a @After method rather than here so that you're sure to stop it even on a test failure.
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/2/tests/custom_cluster/test_automatic_invalidation.py@42
PS2, Line 42: time.sleep(10)
> Do we flush the log then?
I think we already start the daemons with a default of -logbufsecs=1 so the log will automatically flush once a second, rigth?
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Sun, 26 Aug 2018 15:24:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "unused_table_ttl_sec" is invalidated from catalogd.
2. If the old GC generation is more than 70% full, 10% LRU tables are
invalidated. This could be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
unsed during planning.
Tests on time-based invalidation is added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableShrinker.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableShrinkerTest.java
A tests/custom_cluster/test_automatic_invalidation.py
25 files changed, 584 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/2
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation is added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
25 files changed, 788 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/5
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/656/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 00:47:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
This is also an essential feature for the old catalog (2.x branch).
Haven't dug deeper, but I have a question that can we add a whitelist for this eviction? It's expensive to load metadata of some huge tables, so it'd be great if they can always being cached. Otherwise the first unlucky query accessing these table will be stuck for a long time. To be simple, we can let the whitelist configurable only in the startup options.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Fri, 31 Aug 2018 13:47:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 6:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/11224/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11224/4//COMMIT_MSG@19
PS4, Line 19: is
nit: are
http://gerrit.cloudera.org:8080/#/c/11224/4//COMMIT_MSG@20
PS4, Line 20: randomly stuffed into catalogd
what does this mean?
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@210
PS4, Line 210: query
only queries?
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@211
PS4, Line 211: evict
just clarifying that this knob and the memory ones are independent. in other words, if you specify a timeout, tables will still be evicted regardless of how close you get to the memory threshold set below.
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@212
PS4, Line 212: unused
nit: remove
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@225
PS4, Line 225:
I understand this knob to establish a target lower bound for an eviction event. Why is this in terms of table fraction as opposed to a target based on memory fraction (mb or fraction)?
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@226
PS4, Line 226: ========++
remove
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/exec/catalog-op-executor.h@72
PS4, Line 72: table names
nit: simplify to just tables
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2000
PS4, Line 2000: /**
Because of the thrift spec, I was expecting to see usage count being used somewhere. Add a todo to make use of it.
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@46
PS4, Line 46: ize.
stale?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@62
PS4, Line 62:
eviction
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@89
PS4, Line 89: */
add an "_".
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@116
PS4, Line 116: rn new CatalogdTableInvalidator(catalog, invalidateTablesTimeoutSec,
: i
are these two validated anywhere (e.g., [0,1])?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@140
PS4, Line 140: ldPool =
is there somewhat stable site to link to where these names are given, perhaps for different jdk's?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@146
PS4, Line 146: lastObservedGcCount_ = gcbean.getCollectionCount();
why not do this check before setting various class members above?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@175
PS4, Line 175: getLastGcInfo()
just starting here with the doc for this method, I see that null can be returned. I have not checked the rest of the chain following this invocation, but do we want to guard this?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@94
PS4, Line 94: ile (true) {
put this outside of the while loop.
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@656
PS4, Line 656: load
what rule are you using to pair the load with refreshing the last-used-time? for example, why isn't the load in the true branch on L653 also refreshed? generalizing further, should the refresh be part of a Table method rather than at a call-site?
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 06 Sep 2018 02:11:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 4:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/catalog/catalog-util.cc@256
PS3, Line 256: bool TTableName::operator<(const impala::TTableName& that) const {
> This is only for the following map usage in thrift. Renaming is not possibl
Could we make that a list of pairs instead to get around this? This works fine for now, but if for example we at some point added a third field in TTableName (eg a "catalog" level to the naming) we'd have a really hard-to-find bug since it's unlikely anyone would remember to update this comparator. Furthermore, such a bug could cause bad crashes since operator== and operator< would end up inconsistent.
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@209
PS4, Line 209: invalidate_tables_timeout
nit: this should have a suffix like "_sec" or "_s" so it's clear the units
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@218
PS4, Line 218: invalidate_tables_gc_
if the other two flags are hidden, maybe we shouldn't document them? It seems inconsistent to hide them and then mention them in a non-hidden flag.
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@86
PS4, Line 86: /**
: * A callback called after a shrinking pass is triggered. Used for test purposes.
: */
wrong doc
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@112
PS4, Line 112: Preconditions.checkArgument(unusedTableTtlSec >= 0);
add back the message so that if the user hits this, they know which configuration they got wrong
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@141
PS4, Line 141: if (poolName.contains("Old")) {
maybe invert this and 'continue' so you can unnest the rest of the block?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@148
PS4, Line 148: it's not a implementation of
more concise: "it does not implement"
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@209
PS4, Line 209: if (table.isLoaded()) {
style nit: I think easier to read this loop if we invert this: "if (!table.isLoaded()) continue;"
(generally I find it easier to read code when there is less nesting going on, by handling error or "unusual" cases up front in the control flow)
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@211
PS4, Line 211: if (inactivityTime > retireAgeNano) {
same here
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@255
PS4, Line 255: unusedTableTtlNano_
this should also be changed to be more often than the TTL for the same reason as below.
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@260
PS4, Line 260: unusedTableTtlSec
needs update
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@266
PS4, Line 266: error
this should probably be a WARNING -- I think ERROR should be reserved for unrecoverable user-impacting errors, whereas this is a recoverable error. I think we should also ensure that the error message explains the context and what will happen as a result. For example, something like:
LOG.warning("Unexpected exception thrown while attempting to automatically invalidate tables. Will retry in {} seconds", sleepTime, e)
that way if the user sees the message they will know that this doesn't mean that some user query was failed, or that data was lost, or whatever.
Also, I think we should make sure that if an exception is thrown, we still sleep before retrying. Otherwise we might get into some state with a tight loop. Perhaps we should just add a fixed wait of 5 seconds or something here in the catch clause?
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
File fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java@56
PS4, Line 56: .invalidateTable(new TTableName(/*dbName=*/"functional", /*tblName=*/"alltypes"),
sorry, in my previous comment I meant that you can use the 'dbName' and 'tblName' variables that you defined just above.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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, 05 Sep 2018 18:04:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:
http://gerrit.cloudera.org:8080/#/c/11224/1/be/src/catalog/catalog-server.cc@239
PS1, Line 239: webserver->RegisterUrlCallback(INVALIDATE_LRU_TABLE_WEB_PAGE, INVALIDATE_LRU_TABLE_TEMPLATE,
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/11224/1/common/thrift/CatalogService.thrift
File common/thrift/CatalogService.thrift:
http://gerrit.cloudera.org:8080/#/c/11224/1/common/thrift/CatalogService.thrift@424
PS1, Line 424: TInvalidateUnusedTablesResponse InvalidateUnusedTables(1: TInvalidateUnusedTablesRequest req);
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/11224/1/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/1/tests/custom_cluster/test_automatic_invalidation.py@62
PS1, Line 62:
flake8: W391 blank line at end of file
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Aug 2018 18:52:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/352/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 Aug 2018 19:33:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout_s" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation are added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/View.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
28 files changed, 809 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/8
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 8:
(1 comment)
one more clarification.
http://gerrit.cloudera.org:8080/#/c/11224/8/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/8/be/src/common/global-flags.cc@219
PS8, Line 219: DEFINE_int32(invalidate_tables_timeout_s, 0, "If a table has not been referenced in a "
make it clear if this flag must be applied to both impalad coordinators and catalogd. same with the other flags.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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, 11 Sep 2018 17:30:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 4:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/11224/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11224/4//COMMIT_MSG@19
PS4, Line 19: is
> nit: are
Done
http://gerrit.cloudera.org:8080/#/c/11224/4//COMMIT_MSG@20
PS4, Line 20: randomly stuffed into catalogd
> what does this mean?
I wrote a thread that kept creating stringbuilders. The notification listener is called and the memory pool names are as expected.
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@210
PS4, Line 210: query
> only queries?
changed to SQL statements
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@211
PS4, Line 211: evict
> just clarifying that this knob and the memory ones are independent. in othe
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@212
PS4, Line 212: unused
> nit: remove
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@225
PS4, Line 225: invalidate_tables_fraction_on_memory_pressure
> I understand this knob to establish a target lower bound for an eviction ev
We don't know how much memory is released until a GC.
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/common/global-flags.cc@226
PS4, Line 226: the numbers of
> remove
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/4/be/src/exec/catalog-op-executor.h@72
PS4, Line 72: table names
> nit: simplify to just tables
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2000
PS4, Line 2000: * Set the last used time of specified tables to now.
> Because of the thrift spec, I was expecting to see usage count being used s
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@46
PS4, Line 46: unused_tbl_ttl_sec
> stale?
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@62
PS4, Line 62: shrinking
> eviction
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@89
PS4, Line 89: private long lastInvalidationTime;
> add an "_".
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@116
PS4, Line 116: BackendConfig.INSTANCE.getInvalidateTablesGcOldGenFullThreshold(),
: B
> are these two validated anywhere (e.g., [0,1])?
added a check
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@140
PS4, Line 140: poolName
> is there somewhat stable site to link to where these names are given, perha
I can't find any.
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@146
PS4, Line 146: if (!(gcbean instanceof NotificationListener)) {
> why not do this check before setting various class members above?
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@175
PS4, Line 175: getLastGcInfo()
> just starting here with the doc for this method, I see that null can be ret
The call site of this function is guarded with a try clause so I think it's safe
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java@94
PS4, Line 94: Random random = new Random();
> put this outside of the while loop.
Done
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:
http://gerrit.cloudera.org:8080/#/c/11224/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@656
PS4, Line 656: load
> what rule are you using to pair the load with refreshing the last-used-time
It's a mistake. It's now called in load() rather than at the call site.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 06 Sep 2018 23:27:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h
File be/src/exec/catalog-op-executor.h:
http://gerrit.cloudera.org:8080/#/c/11224/3/be/src/exec/catalog-op-executor.h@72
PS3, Line 72: the
: /// numbers of their usages
> nit: their usage frequency
disagree - frequency implies some division, doesn't it?
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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, 27 Aug 2018 20:06:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 9: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3150/
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 02:14:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3156/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 19:18:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout_s" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation are added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/View.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
27 files changed, 829 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/10
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout_s" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation are added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/View.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
28 files changed, 804 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/7
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/609/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 7
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 06 Sep 2018 23:57:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "unused_table_ttl_sec" is invalidated from catalogd.
2. If the old GC generation is almost full, a centen percentage of LRU
tables are invalidated. This could be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
unsed during planning.
Tests on time-based invalidation is added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/Db.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
27 files changed, 734 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/3
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout_s" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation are added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/View.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
27 files changed, 812 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/9
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation is added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
26 files changed, 787 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/4
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/420/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 2
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Tue, 21 Aug 2018 01:23:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout_s" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation are added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Reviewed-on: http://gerrit.cloudera.org:8080/11224
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Tianyi Wang <tw...@cloudera.com>
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
M fe/src/main/java/org/apache/impala/catalog/HBaseTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/KuduTable.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/View.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
27 files changed, 829 insertions(+), 25 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Tianyi Wang: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 12
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 9:
clang-tidy complained about codes I didn't change. I'll fix those warnings.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 9
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 19:14:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 8: Code-Review+2
(4 comments)
several small changes: couple of nits, couple of unused python warnings, and a suggestion for an extra precondition.
http://gerrit.cloudera.org:8080/#/c/11224/8/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
File fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java:
http://gerrit.cloudera.org:8080/#/c/11224/8/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@77
PS8, Line 77: scanning occurred and to proceed
nit, simplify: scan happened.
http://gerrit.cloudera.org:8080/#/c/11224/8/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@189
PS8, Line 189: returns
nit: returned
http://gerrit.cloudera.org:8080/#/c/11224/8/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@189
PS8, Line 189: Will continue without " +
: "invalidating tables based on memory pressure this time.
How about: Table invalidation due to memory pressure was skipped.
http://gerrit.cloudera.org:8080/#/c/11224/8/fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java@193
PS8, Line 193: tenuredGenUsage
from the docs, getMemoryUsageAfterGc should be non-null and given that we found the pool on L163, this should also be non-null. add a precondition for this?
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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, 10 Sep 2018 23:47:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11224/8/tests/custom_cluster/test_automatic_invalidation.py
File tests/custom_cluster/test_automatic_invalidation.py:
http://gerrit.cloudera.org:8080/#/c/11224/8/tests/custom_cluster/test_automatic_invalidation.py@17
PS8, Line 17: import os
flake8: F401 'os' imported but unused
http://gerrit.cloudera.org:8080/#/c/11224/8/tests/custom_cluster/test_automatic_invalidation.py@20
PS8, Line 20: import urllib
flake8: F401 'urllib' imported but unused
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 8
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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, 10 Sep 2018 22:33:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 5:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/597/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 5
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 06 Sep 2018 02:22:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 3:
> Patch Set 3:
>
> This is also an essential feature for the old catalog (2.x branch).
Yep, this is important for both "v2" and "v1". I thought in general the 2.x branch is not getting new features, though.
> Haven't dug deeper, but I have a question that can we add a whitelist for this eviction? It's expensive to load metadata of some huge tables, so it'd be great if they can always being cached. Otherwise the first unlucky query accessing these table will be stuck for a long time. To be simple, we can let the whitelist configurable only in the startup options.
Unfortunately those huge tables also are the ones that take up the most memory, so there is the most benefit for evicting :) I guess a user-configurable way to prevent eviction would be useful but maybe we can add that as a follow-up instead of increasing the scope of this patch? One option might be to add a new table property, so the auto-invalidator skips tables when it sees something like "impala.pin_metadata" = "true"
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 3
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Fri, 31 Aug 2018 17:55:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/668/ : 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/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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: Thu, 13 Sep 2018 19:42:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/579/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 4
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.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, 05 Sep 2018 01:38:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11224/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@261
PS1, Line 261: sleep((long)(unusedTableTtlSec) * 1000L);
TimeUnit.SECONDS.sleep(unusedTableTtlSec);
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@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-Comment-Date: Wed, 15 Aug 2018 20:29:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7448: Invalidate recently unused tables from catalogd
Posted by "Tianyi Wang (Code Review)" <ge...@cloudera.org>.
Tianyi Wang has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11224 )
Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd
......................................................................
IMPALA-7448: Invalidate recently unused tables from catalogd
This patch implements an automatic invalidation mechanism in catalogd.
There are two invalidation strategies:
1. Periodically the HDFS tables that are not used in a configured
period "invalidate_tables_timeout" is invalidated from catalogd.
2. If the old GC generation is almost full, a certain percentage of LRU
tables are invalidated. This can be enabled by backend flag
"invalidate_tables_on_memory_pressure".
The table usage is reported by impalad to catalogd when the tables are
used during planning.
Tests on time-based invalidation is added. It is manually verified that
the GC callback is called if strings are randomly stuffed into catalogd.
Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
---
M be/src/catalog/catalog-server.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/catalog.cc
M be/src/catalog/catalog.h
M be/src/common/global-flags.cc
M be/src/exec/catalog-op-executor.cc
M be/src/exec/catalog-op-executor.h
M be/src/service/fe-support.cc
M be/src/util/backend-gflag-util.cc
M common/thrift/BackendGflags.thrift
M common/thrift/CatalogService.thrift
M fe/src/main/java/org/apache/impala/analysis/StmtMetadataLoader.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
A fe/src/main/java/org/apache/impala/catalog/CatalogdTableInvalidator.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
A fe/src/main/java/org/apache/impala/catalog/ImpaladTableUsageTracker.java
M fe/src/main/java/org/apache/impala/catalog/Table.java
M fe/src/main/java/org/apache/impala/catalog/TableLoader.java
M fe/src/main/java/org/apache/impala/service/BackendConfig.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
A fe/src/test/java/org/apache/impala/catalog/CatalogdTableInvalidatorTest.java
A tests/custom_cluster/test_automatic_invalidation.py
25 files changed, 788 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/11224/6
--
To view, visit http://gerrit.cloudera.org:8080/11224
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib549717abefcffb14d9a3814ee8cf0de8bd49e89
Gerrit-Change-Number: 11224
Gerrit-PatchSet: 6
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>