You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/10/18 06:12:25 UTC

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege 

Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11721


Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled.  types. This patch fixes the issue and
makes the /catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 232 insertions(+), 13 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/2
-- 
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: newchange
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 5:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/1098/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 23:52:26 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1114/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:53:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1090/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 16:09:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+1

Carry +1


-- 
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: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 06:50:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11721/8/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:

http://gerrit.cloudera.org:8080/#/c/11721/8/be/src/catalog/catalog-util.cc@227
PS8, Line 227: 
> lets use StringParser::StringToInt here (util/string-parser.h)
Ah I didn't know about that function. Done.



-- 
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: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:20:03 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 229 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/4
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1093/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 18:42:10 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/1110/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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 <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 19:48:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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 10: Code-Review+2


-- 
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: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:21:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 257 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/8
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+1

(3 comments)

Carry Csaba's +1.

http://gerrit.cloudera.org:8080/#/c/11721/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11721/2//COMMIT_MSG@12
PS2, Line 12: his pa
> Typo?
Oops. Done.


http://gerrit.cloudera.org:8080/#/c/11721/2/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:

http://gerrit.cloudera.org:8080/#/c/11721/2/be/src/catalog/catalog-util.h@104
PS2, Line 104: /// Populates a TPrivilegeLevel::type based on the given object name string.
             : Status TPrivilegeLevelFromObjectName(const std::string& object_name,
             :     TPrivilegeLevel::type* privilege_level);
> Does this have to be in the header? It is only used in catalog-util.cc
It's also used in catalog-util-test.cc.


http://gerrit.cloudera.org:8080/#/c/11721/2/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:

http://gerrit.cloudera.org:8080/#/c/11721/2/tests/authorization/test_authorization.py@468
PS2, Line 468:         assert "catalog_version" in obj_dump
> nit: this does not look as descriptive as 'assert "CatalogException" in obj
Done



-- 
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: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 15:36:03 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11: Code-Review+2


-- 
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: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:26:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1113/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:21:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer 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 2: Code-Review+1

(3 comments)

Just a few minor issues, lgtm otherwise.

http://gerrit.cloudera.org:8080/#/c/11721/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11721/2//COMMIT_MSG@12
PS2, Line 12: types.
Typo?


http://gerrit.cloudera.org:8080/#/c/11721/2/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:

http://gerrit.cloudera.org:8080/#/c/11721/2/be/src/catalog/catalog-util.h@104
PS2, Line 104: /// Populates a TPrivilegeLevel::type based on the given object name string.
             : Status TPrivilegeLevelFromObjectName(const std::string& object_name,
             :     TPrivilegeLevel::type* privilege_level);
Does this have to be in the header? It is only used in catalog-util.cc


http://gerrit.cloudera.org:8080/#/c/11721/2/tests/authorization/test_authorization.py
File tests/authorization/test_authorization.py:

http://gerrit.cloudera.org:8080/#/c/11721/2/tests/authorization/test_authorization.py@468
PS2, Line 468:         service.extract_catalog_object_version(obj_dump)
nit: this does not look as descriptive as 'assert "CatalogException" in obj_dump' - maybe 'assert "CatalogException" not in obj_dump' or 'assert "catalog_version" in obj_dump' would clearer.



-- 
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: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 12:45:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Reviewed-on: http://gerrit.cloudera.org:8080/11721
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 260 insertions(+), 13 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer 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 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11721/2/be/src/catalog/catalog-util.h
File be/src/catalog/catalog-util.h:

http://gerrit.cloudera.org:8080/#/c/11721/2/be/src/catalog/catalog-util.h@104
PS2, Line 104: /// Populates a TPrivilegeLevel::type based on the given object name string.
             : Status TPrivilegeLevelFromObjectName(const std::string& object_name,
             :     TPrivilegeLevel::type* privilege_level);
> It's also used in catalog-util-test.cc.
Hmm, I cannot find the place where it is used.



-- 
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: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 17:16:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3336/ DRY_RUN=false


-- 
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: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:26:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 8:

Fixed clang-tidy.


-- 
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: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:00:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 260 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/10
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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 6:

(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, unexpected lengths, mixed case. as it stands, I don't trust that string (where's it come from?)


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: privilege
principal


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@210
PS6, Line 210: privilege
principal


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@226
PS6, Line 226: atoi(principal_id.c_str())
what does this do for a malformed id?


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@227
PS6, Line 227:       if (principal_type == "ROLE") {
guaranteed to be uppercase?


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@248
PS6, Line 248: TPrivilege* privilege
dcheck not null for this. its a publicly exposed method (for test only?)


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@250
PS6, Line 250:   boost::algorithm::split_regex(split, object_name, boost::regex("->"));
add a comment about the expected format and an example.


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@253
PS6, Line 253: key_value, s, [](char c){ return c == '='; });
             :     if (key_value[0]
check the len... valid?


http://gerrit.cloudera.org:8080/#/c/11721/6/be/src/catalog/catalog-util.cc@254
PS6, Line 254: "server"
all guaranteed to be lowercase?



-- 
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: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 18:38:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 232 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/5
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1100/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 00:30:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:

http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@a214
PS4, Line 214: 
> what is the severity of the issue? ... priv doesn't get shown, crash, somet
privilege doesn't get shown.


http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@271
PS4, Line 271:       Status status = TPrivilegeLevelFromObjectName(key_value[1], &privilege_level);
> is there a comparable parsing method on the java side (frontend)? if so, wo
There is for building the privilege name and not parsing it.


http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@274
PS4, Line 274:     } else if (key_value[0] == "grantoption") {
> const
Done



-- 
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: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 23:22:17 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 6:

Fixed clang-tidy error.


-- 
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: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 00:02:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:

http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@a214
PS4, Line 214: 
what is the severity of the issue? ... priv doesn't get shown, crash, something else?


http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@271
PS4, Line 271: Status TPrivilegeFromObjectName(const string& object_name, TPrivilege* privilege) {
is there a comparable parsing method on the java side (frontend)? if so, would be worthwhile for this to avoid duplication.


http://gerrit.cloudera.org:8080/#/c/11721/4/be/src/catalog/catalog-util.cc@274
PS4, Line 274:   for (auto& s: split) {
const



-- 
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: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 18:40:13 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 232 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/6
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer 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 4: Code-Review+1


-- 
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: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 18:15:45 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1085/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 18 Oct 2018 06:51:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 232 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/3
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac 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 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11721/8/be/src/catalog/catalog-util.cc
File be/src/catalog/catalog-util.cc:

http://gerrit.cloudera.org:8080/#/c/11721/8/be/src/catalog/catalog-util.cc@227
PS8, Line 227: stoi(principal_id))
lets use StringParser::StringToInt here (util/string-parser.h)



-- 
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: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 20:02:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 19 Oct 2018 19:13:07 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11721 )

Change subject: IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
......................................................................

IMPALA-7721: Fix broken /catalog_object web API when getting a privilege

Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.

Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests

Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
---
M be/src/catalog/catalog-util-test.cc
M be/src/catalog/catalog-util.cc
M be/src/catalog/catalog-util.h
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M tests/authorization/test_authorization.py
6 files changed, 257 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/11721/7
-- 
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: newpatchset
Gerrit-Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Gerrit-Change-Number: 11721
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-7721: Fix broken /catalog object web API when getting a privilege

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 11: Verified+1


-- 
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: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 20 Oct 2018 00:19:16 +0000
Gerrit-HasComments: No