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 <[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 23:27:43 +0000 Gerrit-HasComments: Yes
