Vihang Karajgaonkar 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 4: (5 comments) Thanks for making the suggested changes. Left some comments related to initialization logic and default value of the config. http://gerrit.cloudera.org:8080/#/c/14846/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/14846/4/be/src/common/global-flags.cc@297 PS4, Line 297: 16 Do you think we should disable this by default? Since this patch only applies when authorization is enabled, turning this on by default may not be the best use of resources for all the users. Also, the slow access check doesn't affect Ranger based authorization. http://gerrit.cloudera.org:8080/#/c/14846/4/be/src/common/global-flags.cc@299 PS4, Line 299: authentication Can you some more description here related to acceptable values? We also don't want the users to set any unreasonable values (like negative, 0 or too large a number). May be add something like: The number of threads used to check access for the user when executing show tables/databases. This configuration is applicable only when authorization is enabled. A value of 0 or 1 disables multi-threaded execution for checking access. The value must be in the range of 0 to 32. http://gerrit.cloudera.org:8080/#/c/14846/4/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/4/fe/src/main/java/org/apache/impala/service/Frontend.java@273 PS4, Line 273: ExecutorService this can be final http://gerrit.cloudera.org:8080/#/c/14846/4/fe/src/main/java/org/apache/impala/service/Frontend.java@274 PS4, Line 274: BackendConfig.INSTANCE.getNumCheckAccessThreads() Can you initialize this using a private static method instead? That way you can validate the configuration value like say Preconditions.checkState(numThreads >= 0 && numThreads < MAX_CHECK_ACCESS_POOL_SIZE); Also, currently the threadpool will be created no matter if authorization is enabled or not. So may be we should only initialize the pool when authorization is enabled. http://gerrit.cloudera.org:8080/#/c/14846/4/fe/src/main/java/org/apache/impala/service/Frontend.java@808 PS4, Line 808: checkSet nit, may be rename to checkList, since its a list not a set. -- 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: 4 Gerrit-Owner: Zhou Xu <[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: Fri, 13 Dec 2019 19:53:23 +0000 Gerrit-HasComments: Yes
