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 2: (9 comments) I think the patch's approach makes sense in the short term. However, I think a better long term solution would be to only list privileges for a given Authorizable object for the user instead of listing all the privileges for the user. This causes the underlying checkAccess method in Sentry library to be highly inefficient. Currently, I see that the authorizables field is ignored in this method: https://github.com/apache/sentry/blob/branch-2.1.0/sentry-provider/sentry-provider-cache/src/main/java/org/apache/sentry/provider/cache/SimpleCacheProviderBackend.java#L64 What this effectively means is that for each access each, instead of checking against the privileges for that object for the user, we are currently checking against all the privileges for the user. When you add fine-grained privileges it increases the number of privilege checks and I think this is the cause of slowdown. I created IMPALA-9242 which has more details. http://gerrit.cloudera.org:8080/#/c/14846/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/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@762 PS2, Line 762: private String dbName_; : private String tblName_; : private String owner_; : private User user_; change to final variables? http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@767 PS2, Line 767: CheckAccess Can you add some documentation for this class. Specifically, which fields can be null or not. Also, may be add a Preconditions.checkNotNull for dbName, owner and user? http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@774 PS2, Line 774: public nit, Adding @Override makes is more readable in my opinion. http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@797 PS2, Line 797: Remove corresponding elements according to pendingCheckingTasks May be say something like "This method filters out elements from the given list based on the the results of the pendingCheckTasks? Also, can you rename pendingCheckingTasks to pendingCheckTasks in this file? http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@799 PS2, Line 799: void Is this method package-private for a reason? If not, lets keep it a private method. http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@799 PS2, Line 799: filterUnaccessiableElements nit, misspelled method name. Should be "filterUnaccessibleElements" http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@800 PS2, Line 800: list may be use a better name? http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@803 PS2, Line 803: Iterator Can we add a Preconditions check to make sure that the size of list and pendingCheckTasks is the same? http://gerrit.cloudera.org:8080/#/c/14846/2/fe/src/main/java/org/apache/impala/service/Frontend.java@808 PS2, Line 808: iter.remove(); nit, if you want to avoid braces, may be inline this too. Else using braces is more consistent with the coding style conventions we use. -- 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: 2 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 01:14:23 +0000 Gerrit-HasComments: Yes
