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 <[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: Fri, 07 Sep 2018 05:45:19 +0000 Gerrit-HasComments: Yes
