You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by gr...@apache.org on 2019/06/13 17:18:11 UTC

[kudu] branch master updated (349aeaa -> 73285f6)

This is an automated email from the ASF dual-hosted git repository.

granthenke pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git.


    from 349aeaa  KUDU-2846: optimize predicate evaluation for primitives
     new efb648e  KUDU-2850: fix thrift comparison
     new 73285f6  Bump Sentry version to b71a78e

The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 src/kudu/sentry/sentry_common_service.thrift |  2 +-
 src/kudu/sentry/sentry_policy_service.thrift | 21 +++++++++-
 src/kudu/sentry/thrift_operators-test.cc     | 50 ++++++++++++++++++----
 src/kudu/sentry/thrift_operators.cc          | 63 +++++++++++++++++-----------
 thirdparty/vars.sh                           |  6 +--
 5 files changed, 105 insertions(+), 37 deletions(-)


[kudu] 01/02: KUDU-2850: fix thrift comparison

Posted by gr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

granthenke pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit efb648e913452d02496d3cf63f843b8795b7dc12
Author: Andrew Wong <aw...@apache.org>
AuthorDate: Thu Jun 13 00:41:12 2019 -0700

    KUDU-2850: fix thrift comparison
    
    In the operator< implementations, we previously wouldn't return early
    when determining that a field in the LHS was greater than that in the
    RHS, when we should have returned false.
    
    I added a test for each operator I updated that would fail without this
    patch.
    
    Change-Id: I071ccb08a385d6a4bfe407ab05621afe437bc29c
    Reviewed-on: http://gerrit.cloudera.org:8080/13608
    Tested-by: Kudu Jenkins
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
    Reviewed-by: Grant Henke <gr...@apache.org>
---
 src/kudu/sentry/thrift_operators-test.cc | 50 +++++++++++++++++++++----
 src/kudu/sentry/thrift_operators.cc      | 63 ++++++++++++++++++++------------
 2 files changed, 81 insertions(+), 32 deletions(-)

diff --git a/src/kudu/sentry/thrift_operators-test.cc b/src/kudu/sentry/thrift_operators-test.cc
index d4aa070..aa05b44 100644
--- a/src/kudu/sentry/thrift_operators-test.cc
+++ b/src/kudu/sentry/thrift_operators-test.cc
@@ -41,7 +41,7 @@ void AssertCompareRequirements(const T& a, const T& b) {
   }
 }
 
-TEST(ThriftOperatorsTest, TestOperatorLt) {
+TEST(ThriftOperatorsTest, TestRoleOperatorLt) {
   // TSentryRole::operator<
   TSentryRole role_a;
   role_a.__set_roleName("a");
@@ -49,10 +49,16 @@ TEST(ThriftOperatorsTest, TestOperatorLt) {
   TSentryRole role_b;
   role_b.__set_roleName("b");
 
+  TSentryRole role_c;
+  role_c.__set_grantorPrincipal("c");
+
   NO_FATALS(AssertCompareRequirements(role_a, role_b));
-  set<TSentryRole> roles { role_a, role_b };
-  ASSERT_EQ(2, roles.size()) << roles;
+  NO_FATALS(AssertCompareRequirements(role_a, role_c));
+  set<TSentryRole> roles { role_a, role_b, role_c};
+  ASSERT_EQ(3, roles.size()) << roles;
+}
 
+TEST(ThriftOperatorsTest, TestGroupOperatorLt) {
   // TSentryGroup::operator<
   TSentryGroup group_a;
   group_a.__set_groupName("a");
@@ -63,10 +69,13 @@ TEST(ThriftOperatorsTest, TestOperatorLt) {
   NO_FATALS(AssertCompareRequirements(group_a, group_b));
   set<TSentryGroup> groups { group_a, group_b };
   ASSERT_EQ(2, groups.size()) << groups;
+}
 
+TEST(ThriftOperatorsTest, TestPrivilegeOperatorLt) {
   // TSentryPrivilege::operator<
   const string kServer = "server1";
   const string kDatabase = "db1";
+  const string kTable = "tbl1";
 
   TSentryPrivilege db_priv;
   db_priv.__set_serverName(kServer);
@@ -75,7 +84,11 @@ TEST(ThriftOperatorsTest, TestOperatorLt) {
   TSentryPrivilege tbl1_priv;
   tbl1_priv.__set_serverName(kServer);
   tbl1_priv.__set_dbName(kDatabase);
-  tbl1_priv.__set_tableName("tbl1");
+  tbl1_priv.__set_tableName(kTable);
+
+  TSentryPrivilege tbl1_priv_no_db;
+  tbl1_priv_no_db.__set_serverName(kServer);
+  tbl1_priv_no_db.__set_tableName(kTable);
 
   TSentryPrivilege tbl2_priv;
   tbl2_priv.__set_serverName(kServer);
@@ -84,11 +97,16 @@ TEST(ThriftOperatorsTest, TestOperatorLt) {
 
   NO_FATALS(AssertCompareRequirements(db_priv, tbl1_priv));
   NO_FATALS(AssertCompareRequirements(db_priv, tbl2_priv));
+  NO_FATALS(AssertCompareRequirements(db_priv, tbl1_priv_no_db));
   NO_FATALS(AssertCompareRequirements(tbl1_priv, tbl2_priv));
-  set<TSentryPrivilege> privileges { db_priv, tbl1_priv, tbl2_priv };
-  ASSERT_EQ(3, privileges.size()) << privileges;
+  set<TSentryPrivilege> privileges { db_priv, tbl1_priv, tbl2_priv, tbl1_priv_no_db };
+  ASSERT_EQ(4, privileges.size()) << privileges;
+}
 
+TEST(ThriftOperatorsTest, TestAuthorizableOperatorLt) {
   // TSentryAuthorizable::operator<
+  const string kServer = "server1";
+  const string kDatabase = "db1";
   TSentryAuthorizable db_authorizable;
   db_authorizable.__set_server(kServer);
   db_authorizable.__set_db(kDatabase);
@@ -103,10 +121,26 @@ TEST(ThriftOperatorsTest, TestOperatorLt) {
   tbl2_authorizable.__set_db(kDatabase);
   tbl2_authorizable.__set_table("tbl2");
 
+  TSentryAuthorizable server_authorizable;
+  server_authorizable.__set_server("server2");
+
+  TSentryAuthorizable uri_authorizable;
+  uri_authorizable.__set_server(kServer);
+  uri_authorizable.__set_uri("http://uri");
+
+  NO_FATALS(AssertCompareRequirements(server_authorizable, db_authorizable));
+  NO_FATALS(AssertCompareRequirements(uri_authorizable, db_authorizable));
   NO_FATALS(AssertCompareRequirements(db_authorizable, tbl1_authorizable));
   NO_FATALS(AssertCompareRequirements(db_authorizable, tbl2_authorizable));
   NO_FATALS(AssertCompareRequirements(tbl1_authorizable, tbl2_authorizable));
-  set<TSentryAuthorizable> authorizables { db_authorizable, tbl1_authorizable, tbl2_authorizable };
-  ASSERT_EQ(3, authorizables.size()) << authorizables;
+  set<TSentryAuthorizable> authorizables {
+      server_authorizable,
+      uri_authorizable,
+      db_authorizable,
+      tbl1_authorizable,
+      tbl2_authorizable
+  };
+  ASSERT_EQ(5, authorizables.size()) << authorizables;
 }
+
 } // namespace sentry
diff --git a/src/kudu/sentry/thrift_operators.cc b/src/kudu/sentry/thrift_operators.cc
index 8d8624f..b6738ff 100644
--- a/src/kudu/sentry/thrift_operators.cc
+++ b/src/kudu/sentry/thrift_operators.cc
@@ -29,18 +29,30 @@
 
 namespace sentry {
 
-// Evaluates to a true expression if the optional field in 'this' is less than
-// the optional field in 'other', otherwise evaluates to a false expression.
+// Returns true if lhs < rhs, false if lhs > rhs, and passes through if the two
+// are equal.
+#define RETURN_IF_DIFFERENT_LT(lhs, rhs) { \
+  if ((lhs) != (rhs)) { \
+    return (lhs) < (rhs); \
+  } \
+}
+
+// Returns true if the optional field in 'this' is less than the optional field
+// in 'other', and false if greater than. Passes through if the two are equal.
 // Unset fields compare less than set fields.
-#define OPTIONAL_FIELD_LT(field) \
-  (this->__isset.field \
-    ? (other.__isset.field && this->field < other.field) \
-    : other.__isset.field)
+#define OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(field) { \
+  if (this->__isset.field && other.__isset.field) { \
+    RETURN_IF_DIFFERENT_LT(this->field, other.field); \
+  } else if (this->__isset.field || other.__isset.field) { \
+    return other.__isset.field; \
+  } \
+}
 
 bool TSentryRole::operator<(const TSentryRole& other) const {
-  return this->roleName < other.roleName
-      || this->groups < other.groups
-      || this->grantorPrincipal < other.grantorPrincipal;
+  RETURN_IF_DIFFERENT_LT(this->roleName, other.roleName);
+  RETURN_IF_DIFFERENT_LT(this->groups, other.groups);
+  RETURN_IF_DIFFERENT_LT(this->grantorPrincipal, other.grantorPrincipal);
+  return false;
 }
 
 bool TSentryGroup::operator<(const TSentryGroup& other) const {
@@ -48,25 +60,28 @@ bool TSentryGroup::operator<(const TSentryGroup& other) const {
 }
 
 bool TSentryPrivilege::operator<(const TSentryPrivilege& other) const {
-  return this->privilegeScope < other.privilegeScope
-      || this->serverName < other.serverName
-      || OPTIONAL_FIELD_LT(dbName)
-      || OPTIONAL_FIELD_LT(tableName)
-      || OPTIONAL_FIELD_LT(URI)
-      || this->action < other.action
-      || OPTIONAL_FIELD_LT(createTime)
-      || OPTIONAL_FIELD_LT(grantOption)
-      || OPTIONAL_FIELD_LT(columnName);
+  RETURN_IF_DIFFERENT_LT(this->privilegeScope, other.privilegeScope);
+  RETURN_IF_DIFFERENT_LT(this->serverName, other.serverName);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(dbName);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(tableName);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(URI);
+  RETURN_IF_DIFFERENT_LT(this->action, other.action);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(createTime);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(grantOption);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(columnName);
+  return false;
 }
 
 bool TSentryAuthorizable::operator<(const TSentryAuthorizable& other) const {
-  return this->server < other.server
-      || OPTIONAL_FIELD_LT(uri)
-      || OPTIONAL_FIELD_LT(db)
-      || OPTIONAL_FIELD_LT(table)
-      || OPTIONAL_FIELD_LT(column);
+  RETURN_IF_DIFFERENT_LT(this->server, other.server);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(uri);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(db);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(table);
+  OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT(column);
+  return false;
 }
 
-#undef OPTIONAL_FIELD_LT
+#undef OPTIONAL_FIELD_RETURN_IF_DIFFERENT_LT
+#undef RETURN_IF_DIFFERENT_LT
 
 } // namespace sentry


[kudu] 02/02: Bump Sentry version to b71a78e

Posted by gr...@apache.org.
This is an automated email from the ASF dual-hosted git repository.

granthenke pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 73285f6fd1978570144dd0974d85780db2d6478f
Author: Hao Hao <ha...@cloudera.com>
AuthorDate: Mon Jun 10 15:25:34 2019 -0700

    Bump Sentry version to b71a78e
    
    This patch bumps Sentry version to include SENTRY-2522, which is critical
    to improve the performance of ListTables by providing bulk permissions
    fetching.
    
    Change-Id: I4882d41aca0b1c058c1e002613e4629fdaeb543f
    Reviewed-on: http://gerrit.cloudera.org:8080/13598
    Reviewed-by: Adar Dembo <ad...@cloudera.com>
    Tested-by: Kudu Jenkins
    Reviewed-by: Grant Henke <gr...@apache.org>
---
 src/kudu/sentry/sentry_common_service.thrift |  2 +-
 src/kudu/sentry/sentry_policy_service.thrift | 21 ++++++++++++++++++++-
 thirdparty/vars.sh                           |  6 +++---
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/kudu/sentry/sentry_common_service.thrift b/src/kudu/sentry/sentry_common_service.thrift
index a375348..b24bd8a 100644
--- a/src/kudu/sentry/sentry_common_service.thrift
+++ b/src/kudu/sentry/sentry_common_service.thrift
@@ -19,7 +19,7 @@
  */
 
 # DO NOT MODIFY! Copied from
-# https://raw.githubusercontent.com/apache/sentry/505b42e81a9d85c4ebe8db3f48ad7a6e824a5db5/sentry-service/sentry-service-api/src/main/resources/sentry_common_service.thrift
+# https://raw.githubusercontent.com/apache/sentry/b71a78ed960702536b35e1f048dc40dfc79992d4/sentry-service/sentry-service-api/src/main/resources/sentry_common_service.thrift
 #
 # With edits:
 #   - Change cpp namespace to 'sentry' to match the Kudu codebase style.
diff --git a/src/kudu/sentry/sentry_policy_service.thrift b/src/kudu/sentry/sentry_policy_service.thrift
index 0d8c981..148d614 100644
--- a/src/kudu/sentry/sentry_policy_service.thrift
+++ b/src/kudu/sentry/sentry_policy_service.thrift
@@ -19,7 +19,7 @@
  */
 
 # DO NOT MODIFY! Copied from
-# https://raw.githubusercontent.com/apache/sentry/505b42e81a9d85c4ebe8db3f48ad7a6e824a5db5/sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
+# https://raw.githubusercontent.com/apache/sentry/b71a78ed960702536b35e1f048dc40dfc79992d4/sentry-service/sentry-service-api/src/main/resources/sentry_policy_service.thrift
 #
 # With edits:
 #   - Change cpp namespace to 'sentry' to match the Kudu codebase style.
@@ -291,6 +291,19 @@ struct TListSentryPrivilegesByAuthResponse {
 3: optional map<TSentryAuthorizable, TSentryPrivilegeMap> privilegesMapByAuthForUsers
 }
 
+struct TListSentryPrivilegesByAuthUserRequest {
+1: required i32 protocol_version = sentry_common_service.TSENTRY_SERVICE_V2,
+2: required string requestorUserName, # user on whose behalf the request is issued
+3: required set<TSentryAuthorizable> authorizableSet,
+4: required string user
+}
+
+struct TListSentryPrivilegesByAuthUserResponse {
+1: required sentry_common_service.TSentryResponseStatus status,
+# Authorizable to set of privileges map
+2: optional map<TSentryAuthorizable, set<TSentryPrivilege>> privilegesMapByAuth,
+}
+
 # Obtain a config value from the Sentry service
 struct TSentryConfigValueRequest {
 1: required i32 protocol_version = sentry_common_service.TSENTRY_SERVICE_V2,
@@ -445,8 +458,14 @@ service SentryPolicyService
 
   TRenamePrivilegesResponse rename_sentry_privilege(1:TRenamePrivilegesRequest request);
 
+  # List sentry privileges filterted based on a set of authorizables, that
+  # granted to the given user and the given role if present.
   TListSentryPrivilegesByAuthResponse list_sentry_privileges_by_authorizable(1:TListSentryPrivilegesByAuthRequest request);
 
+  # List sentry privileges filterted based on a set of authorizables, that
+  # granted to the given user and the groups the user associated with.
+  TListSentryPrivilegesByAuthUserResponse list_sentry_privileges_by_authorizable_and_user(1:TListSentryPrivilegesByAuthUserRequest request);
+
   TSentryConfigValueResponse get_sentry_config_value(1:TSentryConfigValueRequest request);
 
   # export the mapping data in sentry
diff --git a/thirdparty/vars.sh b/thirdparty/vars.sh
index fd8a82b..2e01115 100644
--- a/thirdparty/vars.sh
+++ b/thirdparty/vars.sh
@@ -231,12 +231,12 @@ HADOOP_VERSION=2.8.5
 HADOOP_NAME=hadoop-$HADOOP_VERSION
 HADOOP_SOURCE=$TP_SOURCE_DIR/$HADOOP_NAME
 
-# TODO(dan): bump to a release version once SENTRY-2371, SENTRY-2440 and SENTRY-2471
-# are published. The SHA below is the current head of the master branch.
+# TODO(dan): bump to a release version once SENTRY-2371, SENTRY-2440, SENTRY-2471
+# and SENTRY-2522 are published. The SHA below is the current head of the master branch.
 # Note: Sentry releases source code only. To build the binary tarball, use `dist`
 # maven profile. For example, `mvn clean install -Pdist`. After a successful build,
 # the tarball will be available under sentry-dist/target.
-SENTRY_VERSION=505b42e81a9d85c4ebe8db3f48ad7a6e824a5db5
+SENTRY_VERSION=b71a78ed960702536b35e1f048dc40dfc79992d4
 SENTRY_NAME=sentry-$SENTRY_VERSION
 SENTRY_SOURCE=$TP_SOURCE_DIR/$SENTRY_NAME