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 <[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 01:35:33 +0000 Gerrit-HasComments: Yes
