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/09/26 00:31:28 UTC
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11509
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
IMPALA-7616: Remove the use of privilege name in TPrivilege
Prior to this patch, privilege name was a field in TPrivilege Thrift
message. The privilege name was constructed from any other fields in
the TPrivilege. This is very error-prone since setting privilege name
prior to setting any other fields can cause a consistency issue.
Another issue is updating a particular field in TPrivilege requires
re-building the privilege name and updating its field. This patch
refactors the code by removing the privilege name field from the
TPrivilege and generates the privilege name as needed.
Testing:
- Ran all FE tests
- Ran authorization E2E tests
Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
14 files changed, 92 insertions(+), 137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/11509/6
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@491
PS6, Line 491: .equalsIgnoreCase
> If the name is always lowered, this isn't necessary.
I was under assumption the privilege name was always lowered and made some code changes but it broke some tests. I guess it's probably best to keep the same behavior.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@a565
PS6, Line 565:
> Why was this removed? Isn't it necessary for the key? There can be multipl
You're right. I'm reverting it.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@525
PS6, Line 525: .equalsIgnoreCase
> Case again.
Same argument as before.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@60
PS6, Line 60: * [ServerName=value]->[DbName=value]->[TableName=value]->[ColumnName=value]->[Action Granted=value]->[Grant Option=value]
> line too long (124 > 90)
Can't fix the format.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@250
PS6, Line 250: .toLowerCase()
> The use of toLowerCase seems inconsistent. Should getName just lowerCase i
Same argument as before.
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 03:28:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 10: Code-Review+2
2 +1s from Csaba and Adam, giving it a +2.
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 23:05:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/806/ : 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/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 04:03:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 9:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11509/7/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:
http://gerrit.cloudera.org:8080/#/c/11509/7/common/thrift/CatalogObjects.thrift@537
PS7, Line 537: /
> My suggestion would be to avoid changing the numbers and leave a gap at 1 (
Done
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@490
PS7, Line 490: String privName = PrincipalPrivilege.buildPrivilegeName(filter);
: if (!privName.equalsIgnoreCase(
: PrincipalPrivilege.buildPrivilegeName(privilege))) {
: continue;
: }
> Building the filter priv name could be moved outside the loop, or with some
I can't move it outside the loop because filter needs to be updated with privilege level from privilege.getPrivilege_level().
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java@524
PS7, Line 524: PrincipalPrivilege privilege = tmpPrincipal.getPrivilege(privilegeName);
: if (privilege != null) {
> This could be replaced with tmpPrincipal.GetPrivilege(privilegeName). It is
Done
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java@562
PS7, Line 562:
> nit: This comment should be with the case PRIVILEGE, and should be name+id.
Done
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1825
PS7, Line 1825: }
> Building the privilege name could be moved outside the lock.
Done
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1850
PS7, Line 1850: " does not exist: " + principalName);
> Building the privilege name could be moved outside the lock.
Done
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@268
PS7, Line 268: // Remove the privileges that no longer exist.
: for (String privilegeName: privilegesToRemove) {
: if (principal.getPrincipalType() == TPrincipalType.ROLE) {
: catalog_.removeRolePrivilege(principal.getName(), privilegeName);
: } else {
: catalog_.removeUserPrivilege(principal.getName(), privilegeName);
: }
: }
: }
> This doesn't look ok to me - we are trying to remove the default privilege.
Indeed this looks like a bug. Good catch! I added a test for this.
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 18:38:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
IMPALA-7616: Remove the use of privilege name in TPrivilege
Prior to this patch, privilege name was a field in TPrivilege Thrift
message. The privilege name was constructed from any other fields in
the TPrivilege. This is very error-prone since setting privilege name
prior to setting any other fields can cause a consistency issue.
Another issue is updating a particular field in TPrivilege requires
re-building the privilege name and updating its field. This patch
refactors the code by removing the privilege name field from the
TPrivilege and generates the privilege name as needed.
Testing:
- Ran all FE tests
- Added a new authorization E2E test
- Ran authorization E2E tests
Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
M tests/authorization/test_grant_revoke.py
15 files changed, 158 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/11509/10
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/818/ : 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/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 19:12:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/803/ : 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/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 01:07:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@60
PS6, Line 60: * [ServerName=value]->[DbName=value]->[TableName=value]->[ColumnName=value]->[Action Granted=value]->[Grant Option=value]
line too long (124 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 00:32:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/823/ : 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/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 23:28:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 11: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Thu, 27 Sep 2018 02:51:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 10:
(1 comment)
Carry Csaba's +1
http://gerrit.cloudera.org:8080/#/c/11509/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1807
PS9, Line 1807: an
> typo: and
Done
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 22:55:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 7:
(6 comments)
The comments are optional with the exception of the one in SentryProxy.java.
http://gerrit.cloudera.org:8080/#/c/11509/7/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:
http://gerrit.cloudera.org:8080/#/c/11509/7/common/thrift/CatalogObjects.thrift@537
PS7, Line 537: 1
My suggestion would be to avoid changing the numbers and leave a gap at 1 (comment out the original member). This is not about compatibility (removing a required field breaks that anyway + it is not a goal for internal thrift structs) but about keeping the history of this file as simple as possible.
pros: can make cherry-picking easier - imagine a commit that adds new field, and someone who tries to backport it to a branch without your change - if the numbers are shifted then to new field will be 12, which will cause conflict on the old branch.
cons: I do not see any, leaving gaps in numbers does not add any overhead as far as I understand thrift
I have seen examples both for replacing numbers and leaving gaps in older commits, so I think that there is no consensus about this in Impala.
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@490
PS7, Line 490: String privName = PrincipalPrivilege.buildPrivilegeName(filter);
: if (!privName.equalsIgnoreCase(
: PrincipalPrivilege.buildPrivilegeName(privilege))) {
: continue;
: }
Building the filter priv name could be moved outside the loop, or with some refactor principal.getPrivilige() could be used for the filtered path.
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java@524
PS7, Line 524: for (PrincipalPrivilege p: tmpPrincipal.getPrivileges()) {
: if (p.getName().equalsIgnoreCase(privilegeName)) {
This could be replaced with tmpPrincipal.GetPrivilege(privilegeName). It is also case insensitive and should be faster.
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1825
PS7, Line 1825: principal.removePrivilege(PrincipalPrivilege.buildPrivilegeName(privilege));
Building the privilege name could be moved outside the lock.
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1850
PS7, Line 1850: return principal.getPrivilege(PrincipalPrivilege.buildPrivilegeName(privSpec));
Building the privilege name could be moved outside the lock.
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/util/SentryProxy.java@268
PS7, Line 268: // Remove the privileges that no longer exist.
: for (String privilegeName: privilegesToRemove) {
: TPrivilege privilege = new TPrivilege();
: if (principal.getPrincipalType() == TPrincipalType.ROLE) {
: catalog_.removeRolePrivilege(principal.getName(), privilege);
: } else {
: catalog_.removeUserPrivilege(principal.getName(), privilege);
: }
: }
This doesn't look ok to me - we are trying to remove the default privilege.
remove*Privilege functions use the privilege's name only, so it would be easy to rewrite those to expect privilege names instead of TPrivilege-s.
+ I am curios if there is any test that would catch such a bug - I am not saying that it should be added in this commit, but it may worth a follow up Jira
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 12:21:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 23:06:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 23:04:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
IMPALA-7616: Remove the use of privilege name in TPrivilege
Prior to this patch, privilege name was a field in TPrivilege Thrift
message. The privilege name was constructed from any other fields in
the TPrivilege. This is very error-prone since setting privilege name
prior to setting any other fields can cause a consistency issue.
Another issue is updating a particular field in TPrivilege requires
re-building the privilege name and updating its field. This patch
refactors the code by removing the privilege name field from the
TPrivilege and generates the privilege name as needed.
Testing:
- Ran all FE tests
- Added a new authorization E2E test
- Ran authorization E2E tests
Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
M tests/authorization/test_grant_revoke.py
15 files changed, 158 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/11509/9
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
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/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
IMPALA-7616: Remove the use of privilege name in TPrivilege
Prior to this patch, privilege name was a field in TPrivilege Thrift
message. The privilege name was constructed from any other fields in
the TPrivilege. This is very error-prone since setting privilege name
prior to setting any other fields can cause a consistency issue.
Another issue is updating a particular field in TPrivilege requires
re-building the privilege name and updating its field. This patch
refactors the code by removing the privilege name field from the
TPrivilege and generates the privilege name as needed.
Testing:
- Ran all FE tests
- Added a new authorization E2E test
- Ran authorization E2E tests
Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Reviewed-on: http://gerrit.cloudera.org:8080/11509
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
M tests/authorization/test_grant_revoke.py
15 files changed, 158 insertions(+), 142 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11509/10/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/11509/10/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@60
PS10, Line 60: * [ServerName=value]->[DbName=value]->[TableName=value]->[ColumnName=value]->[Action Granted=value]->[Grant Option=value]
line too long (124 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 22:56:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@491
PS6, Line 491: .equalsIgnoreCase
If the name is always lowered, this isn't necessary.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@a565
PS6, Line 565:
Why was this removed? Isn't it necessary for the key? There can be multiple privileges with the same principal id.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/catalog/Catalog.java@525
PS6, Line 525: .equalsIgnoreCase
Case again.
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java
File fe/src/main/java/org/apache/impala/util/SentryProxy.java:
http://gerrit.cloudera.org:8080/#/c/11509/6/fe/src/main/java/org/apache/impala/util/SentryProxy.java@250
PS6, Line 250: .toLowerCase()
The use of toLowerCase seems inconsistent. Should getName just lowerCase it?
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 01:57:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
IMPALA-7616: Remove the use of privilege name in TPrivilege
Prior to this patch, privilege name was a field in TPrivilege Thrift
message. The privilege name was constructed from any other fields in
the TPrivilege. This is very error-prone since setting privilege name
prior to setting any other fields can cause a consistency issue.
Another issue is updating a particular field in TPrivilege requires
re-building the privilege name and updating its field. This patch
refactors the code by removing the privilege name field from the
TPrivilege and generates the privilege name as needed.
Testing:
- Ran all FE tests
- Ran authorization E2E tests
Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
M fe/src/main/java/org/apache/impala/util/SentryProxy.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
14 files changed, 94 insertions(+), 130 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/11509/7
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11509/9/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/11509/9/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@60
PS9, Line 60: * [ServerName=value]->[DbName=value]->[TableName=value]->[ColumnName=value]->[Action Granted=value]->[Grant Option=value]
line too long (124 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 18:38:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
File fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java@60
PS7, Line 60: * [ServerName=value]->[DbName=value]->[TableName=value]->[ColumnName=value]->[Action Granted=value]->[Grant Option=value]
line too long (124 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 03:28:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 9: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11509/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/9/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1807
PS9, Line 1807: ad
typo: and
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 21:24:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 22:55:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3228/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 23:06:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7616: Remove the use of privilege name in TPrivilege
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11509 )
Change subject: IMPALA-7616: Remove the use of privilege name in TPrivilege
......................................................................
Patch Set 7: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java
File fe/src/main/java/org/apache/impala/catalog/Catalog.java:
http://gerrit.cloudera.org:8080/#/c/11509/7/fe/src/main/java/org/apache/impala/catalog/Catalog.java@562
PS7, Line 562: //
nit: This comment should be with the case PRIVILEGE, and should be name+id.
--
To view, visit http://gerrit.cloudera.org:8080/11509
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia813dcc7d3872f126865c1f8f37175201a0b10ab
Gerrit-Change-Number: 11509
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@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-Comment-Date: Wed, 26 Sep 2018 04:57:25 +0000
Gerrit-HasComments: Yes