[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 8: (25 comments) http://gerrit.cloudera.org:8080/#/c/3371/8//COMMIT_MSG Commit Message: PS8, Line 7: Check privileges when necessary > Maybe "Remove unnecessary privilege checks when accessing catalog objects" Done PS8, Line 13: user only request database name > The description is a bit misleading. In general, requesting a db from the c I will update this later. I need to think about this. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS8, Line 132: Returns databases using matcher > "Returns all databases that match the pattern of 'matcher'." Similar commen Done PS8, Line 134: matcher must not be null. > You don't enforce that. Add a Preconditions.checkNotNull() above L137 or re Done PS8, Line 172: tablePatternMatcher > Just rename to "matcher". Done PS8, Line 359: Filter candidates.getName() using matcher > "Return all catalog objects from 'candidates' that match the pattern of 'ma Done http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Db.java File fe/src/main/java/com/cloudera/impala/catalog/Db.java: PS8, Line 17: * > Please don't do that. We don't need everything from util. yeah I noticed this.. my IDE changed this.. PS8, Line 427: Returns all functions using fnPatternMatcher > "Returns all functions that match the pattern of 'matcher'. if 'matcher' is Done http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: PS8, Line 592: Match tables in database dbName with tablePatternMatcher and check user's privileges. > "Returns all tables in database 'dbName' that match the pattern of 'matcher Done PS8, Line 595: Throw ImpalaException when dbName is null > Where does this happen? Also when describing the conditions during which an ok. just removed. it happens in getTableNames(dbName, matcher). PS8, Line 598: tablePatternMatcher > Let's rename all these variables to 'matcher' unless there are multiple ins Done PS8, Line 624: List columns = Lists.newArrayList(); : if (columnPatternMatcher == null) return columns; > swap these two lines and return Collections.emptyList(); Done http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: PS8, Line 211: params > Plz don't reference variable names from the body of this function in the fu Done PS8, Line 266: params > Same comment as above. Done http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS8, Line 226: Class contains all information needed by getMetadataOp(). > "Utility class that represents the input parameters of getDbsMetadata() fun Done PS8, Line 287: JDBC search patterns > You may want to add a ref to the PatternMatcher class so that the reader ca Done PS8, Line 287: metadataParams > 'metadataParams'. Done PS8, Line 286: the :* search pattern > "the associated search patterns" Done PS8, Line 289: The return value result.dbs contains the list of databases that satisfy :* the "dbPattern_" search pattern. :* result.tableNames[i] contains the list of tables inside dbs[i] that satisfy :* the "tablePattern_" search pattern. :* result.columns[i][j] contains the list of columns of table[j] in dbs[i] :* that satisfy the search condition "columnPattern_". :* result.functions[i] contains the list of functions inside dbs[i] that :* satisfy the "functionPattern_" search pattern. > You need to update this comment. e.g. "dbPattern_" doesn't even show up in I was referring to members in metadataParams. Done. PS8, Line 348: if (columnPatternMatcher == null) continue; > I don't think that is correct here. You'll never populate the missingTbls u Agree.. I changed the behavior of this function. thanks! http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java: Line 1543: > Maybe also add a call to getDbs where the pattern matches a db for which th Done PS8, Line 1626: same as "%" > Also indicate what that means (.e.g. matches all). Done PS8, Line 1641: Pattern ".*" or "." works as well > Mention what that matches. Here and everywhere else Done PS8, Line 1657: "_" should work > Not very descriptive comment :) Done http://gerrit.cloudera.org:8080/#/c/3371/8/te
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 8: (25 comments) http://gerrit.cloudera.org:8080/#/c/3371/8//COMMIT_MSG Commit Message: PS8, Line 7: Check privileges when necessary Maybe "Remove unnecessary privilege checks when accessing catalog objects" PS8, Line 13: user only request database name The description is a bit misleading. In general, requesting a db from the catalog didn't have the same effect, no? Let's try to improve the description so that it is clear what problem this commit solves. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS8, Line 132: Returns databases using matcher "Returns all databases that match the pattern of 'matcher'." Similar comment for all the other functions. PS8, Line 134: matcher must not be null. You don't enforce that. Add a Preconditions.checkNotNull() above L137 or remove the comment if that condition is enforced in fileCatalogObjectsByPatterrn. Same for any other function that makes that assumption. PS8, Line 172: tablePatternMatcher Just rename to "matcher". PS8, Line 359: Filter candidates.getName() using matcher "Return all catalog objects from 'candidates' that match the pattern of 'matcher'. If 'matcher' doesn't have a pattern, all catalog objects in 'candidates' are returned. The results are sorted in CATALOG_OBJECT_ORDER. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Db.java File fe/src/main/java/com/cloudera/impala/catalog/Db.java: PS8, Line 17: * Please don't do that. We don't need everything from util. PS8, Line 427: Returns all functions using fnPatternMatcher "Returns all functions that match the pattern of 'matcher'. if 'matcher' is null, return an empty list." http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: PS8, Line 592: Match tables in database dbName with tablePatternMatcher and check user's privileges. "Returns all tables in database 'dbName' that match the pattern of 'matcher' and are accessible to the given user. Returns an empty list If 'matcher' is null." PS8, Line 595: Throw ImpalaException when dbName is null Where does this happen? Also when describing the conditions during which an exception is thrown you don't need to be that specific. You may remove this comment altogether, it's not that interesting. PS8, Line 598: tablePatternMatcher Let's rename all these variables to 'matcher' unless there are multiple instances that we need to differentiate. PS8, Line 624: List columns = Lists.newArrayList(); : if (columnPatternMatcher == null) return columns; swap these two lines and return Collections.emptyList(); http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java: PS8, Line 211: params Plz don't reference variable names from the body of this function in the function comment. I had to look inside the function to figure out what params is. PS8, Line 266: params Same comment as above. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS8, Line 226: Class contains all information needed by getMetadataOp(). "Utility class that represents the input parameters of getDbsMetadata() function calls." PS8, Line 286: the :* search pattern "the associated search patterns" PS8, Line 287: metadataParams 'metadataParams'. PS8, Line 287: JDBC search patterns You may want to add a ref to the PatternMatcher class so that the reader can see what a JDBC pattern matcher does. PS8, Line 289: The return value result.dbs contains the list of databases that satisfy :* the "dbPattern_" search pattern. :* result.tableNames[i] contains the list of tables inside dbs[i] that satisfy :* the "tablePattern_" search pattern. :* result.columns[i][j] contains the list of columns of table[j] in dbs[i] :* that satisfy the search condition "columnPattern_". :* result.functions[i] contains the list of functions inside dbs[i] that :* satisfy the "functionPattern_" search pattern. You need to update this comment. e.g. "dbPattern_" doesn't even show up in this function anymore. PS8, Line 348: if (columnPatternMatcher == null) continue; I don't think that is correct here. You'll never populate the missingTbls unless a column pattern is specified. http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/test/jav
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
..
Patch Set 8:
(2 comments)
> (2 comments)
>
> Thanks Huaisi, it looks much better now. Can you please update all
> the comments? I will then do a final review.
Sorry I was on PTO yesterday.
http://gerrit.cloudera.org:8080/#/c/3371/7/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS7, Line 595: * Throw ImpalaException when dbName is null or error calling
user.getShortName() in
:* hasAccess()
:*/
: public List getTableNames(String dbName,
PatternMatcher tablePatternMatcher,
: User user) throws ImpalaException {
: if (tablePatternMatcher == null) return
Collections.emptyList();
: List tblNames = impaladCatalog_.ge
> We are not very consistent with our comments. Maybe remove Javadoc for now
Done
PS7, Line 605: String tblName = iter.next();
: PrivilegeRequest privilegeRequest = new Privi
> You can use this pattern instead here and everywhere else. emptyList() is a
Done
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Huaisi Xu
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has uploaded a new patch set (#8). Change subject: IMPALA-3711: Check privileges when necessary .. IMPALA-3711: Check privileges when necessary Previously 1. impala checks all columns privileges when user only request table name 2. impala checks all table+column privileges when user only request database name This patch prevent checking unnecessary privileges. Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 --- M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/com/cloudera/impala/catalog/Db.java M fe/src/main/java/com/cloudera/impala/service/Frontend.java M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java M testdata/workloads/functional-query/queries/QueryTest/show.test 12 files changed, 380 insertions(+), 122 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/8 -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 8 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 7: (2 comments) Thanks Huaisi, it looks much better now. Can you please update all the comments? I will then do a final review. http://gerrit.cloudera.org:8080/#/c/3371/7/fe/src/main/java/com/cloudera/impala/service/Frontend.java File fe/src/main/java/com/cloudera/impala/service/Frontend.java: PS7, Line 595: * @param dbName database to search for tablePattern :* @param tablePatternMatcher matcher used to match tables in dbName :* @param useruser used to check privileges :* @returna list of tables names filtered by matcher and user has :*accessing privileges :* @throws ImpalaExceptionwhen dbName is null or error calling user.getShortName() :*in hasAccess() We are not very consistent with our comments. Maybe remove Javadoc for now since we don't seem to use it anywhere else in this file. PS7, Line 605: List tblNames = Lists.newArrayList(); : if (tablePatternMatcher == null) return tblNames; You can use this pattern instead here and everywhere else. emptyList() is a bit cheaper to construct and is immutable. if (tablePatternMatcher == null) return List.emptyList(); List tblNames = Lists.newArrayList(); -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 7: I am aware that some comments are outdated.. I will update all comments once you think the code is ok. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has uploaded a new patch set (#7). Change subject: IMPALA-3711: Check privileges when necessary .. IMPALA-3711: Check privileges when necessary Previously 1. impala checks all columns privileges when user only request table name 2. impala checks all table+column privileges when user only request database name This patch prevent checking unnecessary privileges. Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 --- M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/com/cloudera/impala/catalog/Db.java M fe/src/main/java/com/cloudera/impala/service/Frontend.java M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java M testdata/workloads/functional-query/queries/QueryTest/show.test 12 files changed, 343 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/7 -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 7 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: (2 comments) sorry for being slow on this.. :) http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS6, Line 350: fe.getTableNames( : db.getName(), "*", tablePatternMatcher, user) > I don't think there will be any duplicate code. getTableNames that takes as ok I understand.thx Line 547: MetadataOpParams getDbsMetadataParams = MetadataOpParams.MetadataOpParamsBuilder() > ignoreAllColumns(true) is equivalent to hasColumnPattern(false). Irrespecti I see.. thanks! -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS6, Line 350: fe.getTableNames( : db.getName(), "*", tablePatternMatcher, user) > why? that way There will be a lot of duplicated code? we cannot call getTab I don't think there will be any duplicate code. getTableNames that takes as input a string for the pattern will simply create a new HivePatternMatcher and pass it to the second getTableNames function. Line 547: MetadataOpParams getDbsMetadataParams = MetadataOpParams.MetadataOpParamsBuilder() > if someone call this with tableName to be null, then we cannot differentiat ignoreAllColumns(true) is equivalent to hasColumnPattern(false). Irrespective of the actual pattern used (null, "", etc), we set the hasDbPattern and hasTablePattern to true by virtue of calling the equivalent set functions. So in the getDbsMetadata() you can check if you have a column pattern and if not, don't make any calls to Catalog.getColumns(). -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS6, Line 229: catalogName > private member should end with a '_'. Done PS6, Line 230: private boolean ignoreAllDbs; : private boolean ignoreAllTables; : private boolean ignoreAllColumns; : private boolean ignoreAllFunctions; > Instead of having the ignore* members, wouldn't be simpler if you had has[d I think we must have this see example at line547 PS6, Line 246: this. > No need for 'this'. Plz remove. Done PS6, Line 316: getDbsMetadataParams > I think we can just name it metadataParams for simplicity Done Line 327: PatternMatcher schemaPatternMatcher = getDbsMetadataParams.ignoreAllDbs > I should use accesser. Done PS6, Line 342: functionName != null > Why don't you just check if fnPatternMatcher is not null and remove L326? see comment above. PS6, Line 350: fe.getTableNames( : db.getName(), "*", tablePatternMatcher, user) > I thought the interface would be: why? that way There will be a lot of duplicated code? we cannot call getTableNames(String dbName, String pattern, Sring user); in getTableNames(String dbName, PatternMatcher matcher, String user). we have to use most of the code in one another? Line 360: > Also, why can't we check here if we have a non-null columnPatternMatcher an will do Line 547: MetadataOpParams getDbsMetadataParams = MetadataOpParams.MetadataOpParamsBuilder() if someone call this with tableName to be null, then we cannot differentiate that after we call getDbsMetadata if we do not explicit has those ignore*s. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS6, Line 229: catalogName private member should end with a '_'. PS6, Line 230: private boolean ignoreAllDbs; : private boolean ignoreAllTables; : private boolean ignoreAllColumns; : private boolean ignoreAllFunctions; Instead of having the ignore* members, wouldn't be simpler if you had has[db|table|column|function]Pattern members instead? These would be false by default unless the corresponding set* function is called. i.e. setDbPattern wouldn't just set the dbPattern value but also set the hasDbPattern to true. PS6, Line 246: this. No need for 'this'. Plz remove. PS6, Line 316: getDbsMetadataParams I think we can just name it metadataParams for simplicity PS6, Line 342: functionName != null Why don't you just check if fnPatternMatcher is not null and remove L326? PS6, Line 350: fe.getTableNames( : db.getName(), "*", tablePatternMatcher, user) I thought the interface would be: Frontend.getTableNames(String dbName, PatternMatcher matcher, String user) and Frontend.getTableNames(String dbName, String pattern, Sring user); Similarly for the Catalog.getTableNames(). Line 360: Also, why can't we check here if we have a non-null columnPatternMatcher and avoid block L361-369? -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: bump -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: Line 327: PatternMatcher schemaPatternMatcher = getDbsMetadataParams.ignoreAllDbs I should use accesser. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 6: (1 comment) bump http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: Line 226:* Class contains all information needed by getMetadataOp(). Just bump... I can add more comment once you think this is what you want... -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 5: New patch with builder pattern.(not sure if this is what you want @Dimitris) -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has uploaded a new patch set (#6). Change subject: IMPALA-3711: Check privileges when necessary .. IMPALA-3711: Check privileges when necessary Previously 1. impala checks all columns privileges when user only request table name 2. impala checks all table+column privileges when user only request database name This patch prevent checking unnecessary privileges. Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 --- M fe/src/main/java/com/cloudera/impala/catalog/Db.java M fe/src/main/java/com/cloudera/impala/service/Frontend.java M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java M testdata/workloads/functional-query/queries/QueryTest/show.test 6 files changed, 311 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/6 -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 5: Note: my patch does not change the result of any test. I kept the same behavior. The test may be strange but it was there already. see IMPALA-3744. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 5: Updated tests.. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has uploaded a new patch set (#5). Change subject: IMPALA-3711: Check privileges when necessary .. IMPALA-3711: Check privileges when necessary Previously 1. impala checks all columns privileges when user only request table name 2. impala checks all table+column privileges when user only request database name This patch prevent checking unnecessary privileges. Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 --- M fe/src/main/java/com/cloudera/impala/service/Frontend.java M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java M testdata/workloads/functional-query/queries/QueryTest/show.test 5 files changed, 203 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/5 -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 4: will talk offline. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-3711: Check privileges when necessary .. Patch Set 4: (7 comments) I think we can fix the issues in the 2nd patch by modifying the Catalog.filterStringsByPattern() functions. Even though it may be correct, I don't think adding all these extra params in the getDbsMetadata is a good approach. It's really confusing and potentially error prone. Let's talk tomorrow. http://gerrit.cloudera.org:8080/#/c/3371/3//COMMIT_MSG Commit Message: PS3, Line 7: Check privileges when necessary I think this patch solves a bigger problem which is not accessing catalog objects that are not needed during HS2 API calls. PS3, Line 9: Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 : : : : I think we can rephrase that a bit. I think you can simply mention that executing HS2 metadata operations in Impala caused unnecessary accesses of catalog objects, thereby resulting in excess authorization checks. Next, briefly describe how this patch addresses this issue. http://gerrit.cloudera.org:8080/#/c/3371/3/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java: PS3, Line 131: /** :* Returns databases that match dbPattern. See filterStringsByPattern for details of :* the pattern match semantics. :* :* dbPattern may be null (and thus matches everything). :*/ Can you update the comment? PS3, Line 165: /** :* Returns a list of tables in the supplied database that match :* tablePattern. See filterStringsByPattern for details of the pattern match semantics. :* :* dbName must not be null, but tablePattern may be null (and thus matches :* everything). :* :* Table names are returned unqualified. :*/ Update comment. PS3, Line 336: empty string, no strings match. :*/ Update comment and everywhere else there is a change in the function signature. http://gerrit.cloudera.org:8080/#/c/3371/3/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java: PS3, Line 239: If functionName is not null, then only function metadata will be returned. :* If tableName is null, then DbsTablesColumns.tableNames and DbsTablesColumns.columns :* will not be populated. :* If columns is null, then DbsTablesColumns.columns will not be populated. Are these comments up to date? PS3, Line 411: schemaNam nit: database names. I don't think it returns the objects. -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Huaisi Xu has uploaded a new patch set (#4). Change subject: IMPALA-3711: Check privileges when necessary .. IMPALA-3711: Check privileges when necessary Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 --- M fe/src/main/java/com/cloudera/impala/service/Frontend.java M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java 4 files changed, 74 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/4 -- To view, visit http://gerrit.cloudera.org:8080/3371 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Huaisi Xu Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Huaisi Xu
