Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11721 )
Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util-test.cc File be/src/catalog/catalog-util-test.cc: http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util-test.cc@122 PS6, Line 122: TPrivilege privilege; > add more edge cases for these negative tests. for example, empty string, un Done. More tests added. The string comes from: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java#L56-L127 http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc File be/src/catalog/catalog-util.cc: http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210 PS6, Line 210: principal > principal Done http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210 PS6, Line 210: principal > principal Done http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@226 PS6, Line 226: > what does this do for a malformed id? atoi usally returns 0 (not guaranteed) for a malformed ID, which is pretty bad: https://stackoverflow.com/questions/8871711/atoi-how-to-identify-the-difference-between-zero-and-error. I changed it to use std::stoi instead. http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@227 PS6, Line 227: catalog_object->privilege.__set_principal_id(stoi(principal_id)); > guaranteed to be uppercase? Yeah we control this: https://gerrit.cloudera.org/c/11721/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java#570 http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@248 PS6, Line 248: e; > dcheck not null for this. its a publicly exposed method (for test only?) It's for exposed for test only. DCHECK added. http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@250 PS6, Line 250: } > add a comment about the expected format and an example. Done http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@253 PS6, Line 253: : Status TPrivilegeFro > check the len... valid? Oops. Forgot to add the check. Done. http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@254 PS6, Line 254: ectName( > all guaranteed to be lowercase? Yeah, we control this: https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java#L56-L127 -- To view, visit http://gerrit.cloudera.org:8080/11721 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7 Gerrit-Change-Number: 11721 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 19:13:07 +0000 Gerrit-HasComments: Yes
