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 <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 06 Sep 2018 02:11:42 +0000 Gerrit-HasComments: Yes
