Todd Lipcon has posted comments on this change. ( )

Change subject: IMPALA-7448: Invalidate recently unused tables from catalogd

Patch Set 2:

File be/src/catalog/
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
File be/src/common/
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.
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
File be/src/exec/catalog-op-executor.h:
PS2, Line 72:   Status ReportTableUsage(const TReportTableUsageRequest& req,
nit: add some docs
File common/thrift/CatalogService.thrift:
PS2, Line 415: num_usage
nit: I think "usage_count" or "num_usages" would be better
PS2, Line 419:   1: required map<CatalogObjects.TTableName, TTableUsage>  
nit: extra space in here
PS2, Line 423:
nit: no need for empty line
PS2, Line 461:   TReportTableUsageResponse ReportTableUsage(1: 
TReportTableUsageRequest req);
nit: doc
File fe/src/main/java/org/apache/impala/analysis/
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 

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?
File fe/src/main/java/org/apache/impala/catalog/
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'?
File fe/src/main/java/org/apache/impala/catalog/
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?
PS2, Line 1920:       for (TTableName tableName : req.tables.keySet()) {
nit: indentation
PS2, Line 1935: setCatalogdTableShrinker
is this for tests? If so please annotate @VisibleForTesting
File fe/src/main/java/org/apache/impala/catalog/

PS2, Line 23: table
nit: tables
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 
PS2, Line 32:   // Plugable time source for tests. Defined as static to avoid 
nit: use /** javadoc format */ so that IDEs can highlight the docs nicely
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.
PS2, Line 35:   private CatalogServiceCatalog catalog_;
can this be final?
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 
PS2, Line 63:             if (!info.getGcAction().equals("end of major GC")) 
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
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
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.
PS2, Line 89:                       .timedWait(CatalogdTableShrinker.this, 
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
PS2, Line 117: invalidate10percent
think this should be coded more generically to invalidate a provided ratio, and 
have a config for that ratio
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.
PS2, Line 135:       catalog_.invalidateTable(tTableName, tblWasRemoved, 
let's here
PS2, Line 144: retireAgeSecs * 1000000000L
I think using one of the TimeUnit conversion functions is easier to read here
PS2, Line 148:           catalog_.invalidateTable(tTableName, tblWasRemoved, 
File fe/src/main/java/org/apache/impala/catalog/
PS2, Line 26: import org.apache.impala.thrift.TReportTableUsageRequest;
File fe/src/main/java/org/apache/impala/catalog/
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
PS2, Line 117:       ImpaladTableUsageTracker.create();
shouldn't you be assigning this to the member variable?
File fe/src/main/java/org/apache/impala/catalog/
PS2, Line 32: /**
            :  * Report table usage to catalogd.
            :  */
can you expand this javadoc to talk a bit more about the role of this class?
PS2, Line 36: ImpaladCatalog
nit: wrong class name
PS2, Line 43: reportThread
can you give this thread a name?
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.
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 :)
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.
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"
PS2, Line 89:
nit: extra space
File fe/src/main/java/org/apache/impala/service/

do we need to mark usage for all of the other calls in this file?
PS2, Line 728: refreshLastUsedTime
why not just put this in 'load()' since it seems you always call it after load?
File fe/src/test/java/org/apache/impala/catalog/
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?
File tests/custom_cluster/
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
PS2, Line 51:      """IMPALA-5789: Test that always false filter filters out 
splits when file-level
            :         filtering is disabled."""
? same above
PS2, Line 60:   time.sleep(20)
            :         assert metadata_cache_string not in 
similarly, to avoid test timing dependency, would be better to loop here and 
check until it succeeds (with a long timeout)

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Andrew Sherman <>
Gerrit-Reviewer: Impala Public Jenkins <>
Gerrit-Reviewer: Tianyi Wang <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-Comment-Date: Tue, 21 Aug 2018 01:42:25 +0000
Gerrit-HasComments: Yes

Reply via email to