Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8958 )
Change subject: IMPALA-5152: Introduce metadata loading phase ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/8958/2/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/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@2385 PS2, Line 2385: given name from object for the 'loadedTables' map : * in the global analysis state > Weird sentence. I think you need to remove "object for". Done http://gerrit.cloudera.org:8080/#/c/8958/2/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/2/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@63 PS2, Line 63: public void collectTableRefs(List<TableRef> tblRefs) { : } > nit: single line Done http://gerrit.cloudera.org:8080/#/c/8958/2/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/2/fe/src/main/java/org/apache/impala/service/Frontend.java@877 PS2, Line 877: is > remove Done http://gerrit.cloudera.org:8080/#/c/8958/2/fe/src/main/java/org/apache/impala/service/Frontend.java@898 PS2, Line 898: currCatalog > Is is possible that in the case of a catalog restart, some of the tables th That's not a crazy thought at all. I considered this scenario and believe it will work for the following reasons: * A Table added to the loadedTbls map reflects the table loaded at some point in time. The requestTableLoadAndWait() process is strictly additive, i.e., new loaded tables may be added, but an existing loaded table is never removed, even if its current state in the impalad catalog is "unloaded". * Tables on the impalad side are not modified in place. This means that a Table in the loadedTbls will always remain in the loaded state. * Query compilation only used Tables from the StmtTableCache, never from the impalad's catalog cache. Modified function comment to explain. http://gerrit.cloudera.org:8080/#/c/8958/2/fe/src/main/java/org/apache/impala/service/Frontend.java@936 PS2, Line 936: loadedTables > loadedTbls Done http://gerrit.cloudera.org:8080/#/c/8958/2/fe/src/main/java/org/apache/impala/service/Frontend.java@987 PS2, Line 987: Sets.newHashSet(tableNames); > Why not just "return tableNames"? Good point. Done. http://gerrit.cloudera.org:8080/#/c/8958/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1118 PS2, Line 1118: Planner > nit: Eventually we may want to rename to something like "Query Compilation" I agree. Changed to Query Compliation. -- 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: 2 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: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Feb 2018 19:08:02 +0000 Gerrit-HasComments: Yes
