Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14846 )
Change subject: IMPALA-9195: Using multithreaded execution to accelerate 'show tables/databases' ...................................................................... Patch Set 11: (8 comments) This patch looks good to me! Leave some comments about error handling and nits. http://gerrit.cloudera.org:8080/#/c/14846/11/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/14846/11/be/src/common/global-flags.cc@306 PS11, Line 306: A value of 1 disables multi-threaded execution for checking authorization. It may worth to mention that when set larger than 1, the parallism of FE requests may be limit by this. So the admin won't set this to 2 or 3 hoping SHOW statements get 2-3 times speed-up. http://gerrit.cloudera.org:8080/#/c/14846/11/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/14846/11/fe/src/main/java/org/apache/impala/service/Frontend.java@202 PS11, Line 202: 32 I think 32 is not large enough since the default value of fe_service_threads is 64. So there're at most 128 parallel requests (64 for beeswax, 64 for hs2). Maybe 128 is more reasonable. Looks like no harm to set it larger. http://gerrit.cloudera.org:8080/#/c/14846/11/fe/src/main/java/org/apache/impala/service/Frontend.java@806 PS11, Line 806: /* dbName and user cannot be null, tblName and owner can be null. */ Method comments start by '/**', end by '*/' and explain what the method does. This seems not a method comment. Maybe put it inside the method? http://gerrit.cloudera.org:8080/#/c/14846/11/fe/src/main/java/org/apache/impala/service/Frontend.java@854 PS11, Line 854: nit: don't need space http://gerrit.cloudera.org:8080/#/c/14846/11/fe/src/main/java/org/apache/impala/service/Frontend.java@856 PS11, Line 856: nit: don't need space http://gerrit.cloudera.org:8080/#/c/14846/11/fe/src/main/java/org/apache/impala/service/Frontend.java@857 PS11, Line 857: nit: don't need space http://gerrit.cloudera.org:8080/#/c/14846/11/fe/src/main/java/org/apache/impala/service/Frontend.java@857 PS11, Line 857: LOG.error("Encountered an error checking access" , e); Should we break the loop in this case? http://gerrit.cloudera.org:8080/#/c/14846/11/tests/authorization/test_authorization.py File tests/authorization/test_authorization.py: http://gerrit.cloudera.org:8080/#/c/14846/11/tests/authorization/test_authorization.py@674 PS11, Line 674: test_sentry_show_stmts_with_any It'd be better to use another log folder. This is used in a previous test. -- To view, visit http://gerrit.cloudera.org:8080/14846 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I860e0d18afa0421665f8b3b1c5561d6bdacc5e96 Gerrit-Change-Number: 14846 Gerrit-PatchSet: 11 Gerrit-Owner: Zhou Xu <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Zhou Xu <[email protected]> Gerrit-Comment-Date: Tue, 24 Dec 2019 06:44:22 +0000 Gerrit-HasComments: Yes
