Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8958 )
Change subject: IMPALA-5152: Introduce metadata loading phase ...................................................................... Patch Set 3: (22 comments) I did not rebase for now to make reviewing easier. Still need to rebase and run the full test suite. Should be ok to review the big changes. http://gerrit.cloudera.org:8080/#/c/8958/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8958/3//COMMIT_MSG@62 PS3, Line 62: Testing: : - A core/hdfs run succeeded : - No new tests were added because the existing functional tests : provide good coverage of various metadata loading scenarios. : - The issue reported in IMPALA-5152 is basically impossible now. : Adding FE unit tests for that bug specifically would require : ugly changes to the new code to enable such testing. > Your view of these tests is obviously much deeper than mine. We expect that * Reworked the loading code into a new class, StmtMetadataLoader * Added FE unit test for the StmtMetadataLoader * The tests cover basic and interesting cases, but are not exhaustive (I don't think that's required or useful) http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@460 PS3, Line 460: // Process statements for which column-level privilege requests may be registered : // except for DESCRIBE TABLE, REFRESH/INVALIDATE, USE or SHOW TABLES statem > stale comment? Done http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@462 PS3, Line 462: isResult_.isQueryStmt() || analysisResult_.isInsertStmt() || : analysisResult_.isUpdateStmt() || analysisResult_.isDeleteStmt() || : analysi > worth grouping into a hasColumnPrivilege method? Done http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@508 PS3, Line 508: sult_.isDescribeTableStmt() || : analysisResult_.isResetMetadataStmt() || : analysisResult_.isUseStmt() || : analysisResult_.isShowTablesStmt() || : analysisResult_ > what's special about these? how would one know that altertable belongs here Added comment. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@309 PS3, Line 309: // the map was populated. > is that TODO about other catalog objects still relevant? I think so, but I don't think a TODO here in the code will make us more likely to address it. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/LimitElement.java File fe/src/main/java/org/apache/impala/analysis/LimitElement.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/LimitElement.java@124 PS3, Line 124: || expr.contains(Subquery.cl > comment is stale, pls update. Done http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Path.java File fe/src/main/java/org/apache/impala/analysis/Path.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/Path.java@284 PS3, Line 284: public static List<TableName> getCandidateTables(List<String> path, String sessionDb) { > This seems easy to unit test. Would it make sense to do so? There are ~800 lines of test code dedicated to path analysis in AnalyzeStmts.java. Those tests exercise this function through the metadata loading code path and through the analysis code path. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowFilesStmt.java@62 PS3, Line 62: tableName_.toPath() > add a precondition in the constructor to check that this is not null. Done http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/ShowStatsStmt.java@71 PS3, Line 71: tableName_.toPath() > check not-null in constructor. Done http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@61 PS3, Line 61: * Subclasses should override this method as necessary. > Is it possible that someone will forget to do so in the future in an unplea That was my initial implementation. Dimitris suggested I save the code. I'm fine with either approach. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@231 PS3, Line 231: stmt.collectTableRefs(tblRefs, true); > I don't know what the usual style here is, but you might want to consider: I introduced collectFromClauseTableRefs(). It makes some things better and some things worse, you can take a look. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@827 PS3, Line 827: public static final class StmtTableCache { > javadoc? * Added comment. * Depends on your definition of cache I suppose. This thing is "the catalog" used within a single statement. Statement analysis can consult this thing multiple times an for each table we are guaranteed to see the same Table (and in the same state since we never modify a Table in place). Each entry in this map is a loaded table that existed in the ImpaladCatalog at some point in time. The tables are not a consistent snapshot of the ImpaladCatalog. Happy to rename if you have a better suggestion. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@866 PS3, Line 866: final long debugLoggingFrequency = 10; > nit: rename to include units? (e.g., debugLoggingFrequencyMillis). Renamed and added comment. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@894 PS3, Line 894: // Loop until all the missing tables are loaded in the Impalad's catalog cache. : // In every iteration of this loop we wait for one catalog update to arrive. : while (!missingTbls.isEmpty()) { > Is there a limit to the number of iterations we're willing to do? Similarly Fair question. Several points: It is extremely hard to put a "reasonable" bound on the metadata loading time because: * Many details such as the statestore topic update frequency as well as the number of metadata loading threads and various timeouts are configurable in other services. * Queries can vary dramatically in complexity. Even queries with a similar complexity can have dramatically different loading times. * Loading times can differ due to difference in filesystmes (e.g. S3 vs. HDFS), or due to random congestion in the network or transient issues in dependent services (HMS, Kerberos, HDFS, etc.) The behavior before this patch was to wait forever. We have seen rare issues with that in the past but not enough for me to say we need to address that concern in this patch since I don't think there is a simple and effective solution. I think it is safer to preserve the existing behavior as much as possible. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@906 PS3, Line 906: LOG.warn("Catalog restart detected while waiting for table metadata."); > For debugging, would it be useful to log the old and new versions of the ca Good idea. Done. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@928 PS3, Line 928: // 3) Periodically retry to avoid a hang due to anomalies/bugs, e.g., > Do you feel that this case requires more explicit logging? Done http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@959 PS3, Line 959: tbl = catalog.getTable(tblName.getDb(), tblName.getTbl()); > There's a "throw on error" argument to getTable(). Should that be used inst I cleaned up the getTables() calls as well as our local/fast metadata loading path for unit tests. This should be clearer now. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@961 PS3, Line 961: DatabaseNotFoundException > fwict, this is the exception that we were previously using to give back an It's not quite that simple because we are missing context here. For example, we don't know whether the current TableName is part of a longer path (tbl.db.col.nested_coll) where reporting "do does not exist" does not make sense. I made the changes to preserve the nicer error messages. You can take a look. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@960 PS3, Line 960: } catch (CatalogException e) { : Preconditions.checkState(e instanceof DatabaseNotFoundException); > is this just If I catch DatabaseNotFoundException then I still have CatalogException to deal with. I want to avoid throwing since that spreads like cancer. The right fix is outlined in the comment above. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@964 PS3, Line 964: if (tbl == null) continue; > Is there a chance that not recording tbl==null causes extra round trips? We only load tables that are known to exist in an unloaded state. Checking for the existence of a table is a fully local operation against the ImpaladCatalog. Discovering tables requires an invalidate metadata. Table discovery is not part of this metadata loading process. http://gerrit.cloudera.org:8080/#/c/8958/3/fe/src/main/java/org/apache/impala/service/Frontend.java@969 PS3, Line 969: Tbls.put(tblName, t > so we can have views that are marked as loaded, but their base tables may n Yes. Loading a view entails fetching the column definitions and the view definition from the HMS. The loading of the view itself does not include loading all dependent objects. There is no dependency tracking among views and databases/tables/columns. If you rename a table that is referenced in a view, then that view will break. http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java: http://gerrit.cloudera.org:8080/#/c/8958/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java@104 PS1, Line 104: Table does > My guess is that the case you refer to is the common case. If the error is Another point to consider is that when authorization with Sentry is enabled users will most likely get an auth error and not an analysis error. Unless, of course, the user happens to have privileges on that non-existent database or is an admin. I made the changes to preserve the nicer error messages. You can take a look. -- To view, visit http://gerrit.cloudera.org:8080/8958 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I68d32d5acd4a6f6bc6cedb05e6cc5cf604d24a55 Gerrit-Change-Number: 8958 Gerrit-PatchSet: 3 Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 15 Feb 2018 19:01:12 +0000 Gerrit-HasComments: Yes
