You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by tm...@apache.org on 2018/08/20 20:36:04 UTC
[1/4] impala git commit: IMPALA-6989: Implement SHOW GRANT USER
statement
Repository: impala
Updated Branches:
refs/heads/master 0a901fcb2 -> d29300281
IMPALA-6989: Implement SHOW GRANT USER statement
This patch implements SHOW GRANT USER command to show the list of
privileges for a given user.
Syntax:
SHOW GRANT USER <user>
SHOW GRANT USER <user> ON SERVER
SHOW GRANT USER <user> ON DATABASE <db>
SHOW GRANT USER <user> ON TABLE <table>
SHOW GRANT USER <user> ON URI <uri>
Testing:
- Add FE tests
- Ran all FE tests
Change-Id: Ia96fcf02d249501c471d03511c22625ae2fec225
Reviewed-on: http://gerrit.cloudera.org:8080/11244
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/73d37d6e
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/73d37d6e
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/73d37d6e
Branch: refs/heads/master
Commit: 73d37d6ebbccba7045fff2f228f7364cc1302582
Parents: 0a901fc
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Wed Jul 25 17:35:52 2018 -0500
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Aug 20 01:50:54 2018 +0000
----------------------------------------------------------------------
be/src/service/client-request-state.cc | 6 +-
be/src/service/frontend.cc | 6 +-
be/src/service/frontend.h | 7 +-
common/thrift/Frontend.thrift | 21 +++--
fe/src/main/cup/sql-parser.cup | 42 ++++++---
.../apache/impala/analysis/AnalysisContext.java | 6 +-
.../impala/analysis/ShowGrantPrincipalStmt.java | 95 ++++++++++++++++++++
.../impala/analysis/ShowGrantRoleStmt.java | 86 ------------------
.../impala/catalog/AuthorizationPolicy.java | 24 +----
.../org/apache/impala/service/Frontend.java | 17 ++--
.../org/apache/impala/service/JniFrontend.java | 17 ++--
.../impala/analysis/AnalyzeAuthStmtsTest.java | 33 ++++---
.../impala/analysis/AuthorizationStmtTest.java | 16 ++--
.../org/apache/impala/analysis/ParserTest.java | 35 ++++----
14 files changed, 223 insertions(+), 188 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/be/src/service/client-request-state.cc
----------------------------------------------------------------------
diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index 5839e9c..0db511d 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -346,8 +346,8 @@ Status ClientRequestState::ExecLocalCatalogOp(
SetResultSet(result.role_names);
return Status::OK();
}
- case TCatalogOpType::SHOW_GRANT_ROLE: {
- const TShowGrantRoleParams& params = catalog_op.show_grant_role_params;
+ case TCatalogOpType::SHOW_GRANT_PRINCIPAL: {
+ const TShowGrantPrincipalParams& params = catalog_op.show_grant_principal_params;
if (params.is_admin_op) {
// Verify the user has privileges to perform this operation by checking against
// the Sentry Service (via the Catalog Server).
@@ -361,7 +361,7 @@ Status ClientRequestState::ExecLocalCatalogOp(
}
TResultSet response;
- RETURN_IF_ERROR(frontend_->GetRolePrivileges(params, &response));
+ RETURN_IF_ERROR(frontend_->GetPrincipalPrivileges(params, &response));
// Set the result set and its schema from the response.
request_result_set_.reset(new vector<TResultRow>(response.rows));
result_metadata_ = response.schema;
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/be/src/service/frontend.cc
----------------------------------------------------------------------
diff --git a/be/src/service/frontend.cc b/be/src/service/frontend.cc
index ca3f79f..aec92f7 100644
--- a/be/src/service/frontend.cc
+++ b/be/src/service/frontend.cc
@@ -93,7 +93,7 @@ Frontend::Frontend() {
{"getFunctions", "([B)[B", &get_functions_id_},
{"getCatalogObject", "([B)[B", &get_catalog_object_id_},
{"getRoles", "([B)[B", &show_roles_id_},
- {"getRolePrivileges", "([B)[B", &get_role_privileges_id_},
+ {"getPrincipalPrivileges", "([B)[B", &get_principal_privileges_id_},
{"execHiveServer2MetadataOp", "([B)[B", &exec_hs2_metadata_op_id_},
{"setCatalogIsReady", "()V", &set_catalog_is_ready_id_},
{"waitForCatalog", "()V", &wait_for_catalog_id_},
@@ -186,9 +186,9 @@ Status Frontend::GetStats(const TShowStatsParams& params,
return JniUtil::CallJniMethod(fe_, get_stats_id_, params, result);
}
-Status Frontend::GetRolePrivileges(const TShowGrantRoleParams& params,
+Status Frontend::GetPrincipalPrivileges(const TShowGrantPrincipalParams& params,
TResultSet* result) {
- return JniUtil::CallJniMethod(fe_, get_role_privileges_id_, params, result);
+ return JniUtil::CallJniMethod(fe_, get_principal_privileges_id_, params, result);
}
Status Frontend::GetFunctions(TFunctionCategory::type fn_category, const string& db,
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/be/src/service/frontend.h
----------------------------------------------------------------------
diff --git a/be/src/service/frontend.h b/be/src/service/frontend.h
index 2327e8f..3788fa5 100644
--- a/be/src/service/frontend.h
+++ b/be/src/service/frontend.h
@@ -92,8 +92,9 @@ class Frontend {
/// Call FE to get the table/column stats.
Status GetStats(const TShowStatsParams& params, TResultSet* result);
- /// Call FE to get the privileges granted to a role.
- Status GetRolePrivileges(const TShowGrantRoleParams& params, TResultSet* result);
+ /// Call FE to get the privileges granted to a principal.
+ Status GetPrincipalPrivileges(const TShowGrantPrincipalParams& params,
+ TResultSet* result);
/// Return all functions of 'category' that match the optional argument 'pattern'.
/// If pattern is NULL match all functions, otherwise match only those functions that
@@ -208,7 +209,7 @@ class Frontend {
jmethodID get_functions_id_; // JniFrontend.getFunctions
jmethodID get_catalog_object_id_; // JniFrontend.getCatalogObject
jmethodID show_roles_id_; // JniFrontend.getRoles
- jmethodID get_role_privileges_id_; // JniFrontend.getRolePrivileges
+ jmethodID get_principal_privileges_id_; // JniFrontend.getPrincipalPrivileges
jmethodID exec_hs2_metadata_op_id_; // JniFrontend.execHiveServer2MetadataOp
jmethodID load_table_data_id_; // JniFrontend.loadTableData
jmethodID set_catalog_is_ready_id_; // JniFrontend.setCatalogIsReady
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/common/thrift/Frontend.thrift
----------------------------------------------------------------------
diff --git a/common/thrift/Frontend.thrift b/common/thrift/Frontend.thrift
index abb2d77..a7d56d7 100644
--- a/common/thrift/Frontend.thrift
+++ b/common/thrift/Frontend.thrift
@@ -265,20 +265,23 @@ struct TShowRolesResult {
1: required list<string> role_names
}
-// Parameters for SHOW GRANT ROLE commands
-struct TShowGrantRoleParams {
+// Parameters for SHOW GRANT ROLE/USER commands
+struct TShowGrantPrincipalParams {
// The effective user who submitted this request.
1: optional string requesting_user
- // The target role name.
- 2: required string role_name
+ // The target name.
+ 2: required string name
+
+ // The principal type.
+ 3: required CatalogObjects.TPrincipalType principal_type;
// True if this operation requires admin privileges on the Sentry Service (when
// the requesting user has not been granted the target role name).
- 3: required bool is_admin_op
+ 4: required bool is_admin_op
// An optional filter to show grants that match a specific privilege spec.
- 4: optional CatalogObjects.TPrivilege privilege
+ 5: optional CatalogObjects.TPrivilege privilege
}
// Arguments to getFunctions(), which returns a list of non-qualified function
@@ -436,7 +439,7 @@ enum TCatalogOpType {
SHOW_CREATE_TABLE,
SHOW_DATA_SRCS,
SHOW_ROLES,
- SHOW_GRANT_ROLE,
+ SHOW_GRANT_PRINCIPAL,
SHOW_FILES,
SHOW_CREATE_FUNCTION
}
@@ -473,8 +476,8 @@ struct TCatalogOpRequest {
// Parameters for SHOW ROLES
10: optional TShowRolesParams show_roles_params
- // Parameters for SHOW GRANT ROLE
- 11: optional TShowGrantRoleParams show_grant_role_params
+ // Parameters for SHOW GRANT ROLE/USER
+ 11: optional TShowGrantPrincipalParams show_grant_principal_params
// Parameters for DDL requests executed using the CatalogServer
// such as CREATE, ALTER, and DROP. See CatalogService.TDdlExecRequest
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/cup/sql-parser.cup
----------------------------------------------------------------------
diff --git a/fe/src/main/cup/sql-parser.cup b/fe/src/main/cup/sql-parser.cup
index 94b69e4..e334591 100644
--- a/fe/src/main/cup/sql-parser.cup
+++ b/fe/src/main/cup/sql-parser.cup
@@ -54,6 +54,7 @@ import org.apache.impala.thrift.TOwnerType;
import org.apache.impala.thrift.TPrivilegeLevel;
import org.apache.impala.thrift.TShowStatsOp;
import org.apache.impala.thrift.TTablePropertyType;
+import org.apache.impala.thrift.TPrincipalType;
parser code {:
private Symbol errorToken_;
@@ -497,7 +498,8 @@ nonterminal Map<Option, Object> column_options_map;
// For GRANT/REVOKE/AUTH DDL statements
nonterminal ShowRolesStmt show_roles_stmt;
-nonterminal ShowGrantRoleStmt show_grant_role_stmt;
+nonterminal ShowGrantPrincipalStmt show_grant_principal_stmt;
+nonterminal TPrincipalType principal_type;
nonterminal CreateDropRoleStmt create_drop_role_stmt;
nonterminal GrantRevokeRoleStmt grant_role_stmt;
nonterminal GrantRevokeRoleStmt revoke_role_stmt;
@@ -655,8 +657,8 @@ stmt ::=
{: RESULT = set; :}
| show_roles_stmt:show_roles
{: RESULT = show_roles; :}
- | show_grant_role_stmt:show_grant_role
- {: RESULT = show_grant_role; :}
+ | show_grant_principal_stmt:show_grant_principal
+ {: RESULT = show_grant_principal; :}
| create_drop_role_stmt:create_drop_role
{: RESULT = create_drop_role; :}
| grant_role_stmt:grant_role
@@ -903,29 +905,31 @@ show_roles_stmt ::=
{: RESULT = new ShowRolesStmt(true, null); :}
;
-show_grant_role_stmt ::=
- KW_SHOW KW_GRANT KW_ROLE ident_or_default:role
- {: RESULT = new ShowGrantRoleStmt(role, null); :}
- | KW_SHOW KW_GRANT KW_ROLE ident_or_default:role KW_ON server_ident:server_kw
+show_grant_principal_stmt ::=
+ KW_SHOW KW_GRANT principal_type:type ident_or_default:name
+ {: RESULT = new ShowGrantPrincipalStmt(name, type, null); :}
+ | KW_SHOW KW_GRANT principal_type:type ident_or_default:name KW_ON
+ server_ident:server_kw
{:
- RESULT = new ShowGrantRoleStmt(role,
+ RESULT = new ShowGrantPrincipalStmt(name, type,
PrivilegeSpec.createServerScopedPriv(TPrivilegeLevel.ALL));
:}
- | KW_SHOW KW_GRANT KW_ROLE ident_or_default:role KW_ON
+ | KW_SHOW KW_GRANT principal_type:type ident_or_default:name KW_ON
KW_DATABASE ident_or_default:db_name
{:
- RESULT = new ShowGrantRoleStmt(role,
+ RESULT = new ShowGrantPrincipalStmt(name, type,
PrivilegeSpec.createDbScopedPriv(TPrivilegeLevel.ALL, db_name));
:}
- | KW_SHOW KW_GRANT KW_ROLE ident_or_default:role KW_ON KW_TABLE table_name:tbl_name
+ | KW_SHOW KW_GRANT principal_type:type ident_or_default:name KW_ON KW_TABLE
+ table_name:tbl_name
{:
- RESULT = new ShowGrantRoleStmt(role,
+ RESULT = new ShowGrantPrincipalStmt(name, type,
PrivilegeSpec.createTableScopedPriv(TPrivilegeLevel.ALL, tbl_name));
:}
- | KW_SHOW KW_GRANT KW_ROLE ident_or_default:role KW_ON uri_ident:uri_kw
+ | KW_SHOW KW_GRANT principal_type:type ident_or_default:name KW_ON uri_ident:uri_kw
STRING_LITERAL:uri
{:
- RESULT = new ShowGrantRoleStmt(role,
+ RESULT = new ShowGrantPrincipalStmt(name, type,
PrivilegeSpec.createUriScopedPriv(TPrivilegeLevel.ALL, new HdfsUri(uri)));
:}
;
@@ -991,6 +995,16 @@ privilege ::=
{: RESULT = TPrivilegeLevel.ALL; :}
;
+principal_type ::=
+ KW_ROLE
+ {: RESULT = TPrincipalType.ROLE; :}
+ | IDENT:user
+ {:
+ parser.checkIdentKeyword("USER", user);
+ RESULT = TPrincipalType.USER;
+ :}
+ ;
+
opt_grantopt_for ::=
KW_GRANT option_ident:option KW_FOR
{: RESULT = true; :}
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java b/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
index 47fe8a7..2f51c71 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
@@ -137,7 +137,9 @@ public class AnalysisContext {
public boolean isResetMetadataStmt() { return stmt_ instanceof ResetMetadataStmt; }
public boolean isExplainStmt() { return stmt_.isExplain(); }
public boolean isShowRolesStmt() { return stmt_ instanceof ShowRolesStmt; }
- public boolean isShowGrantRoleStmt() { return stmt_ instanceof ShowGrantRoleStmt; }
+ public boolean isShowGrantPrincipalStmt() {
+ return stmt_ instanceof ShowGrantPrincipalStmt;
+ }
public boolean isCreateDropRoleStmt() { return stmt_ instanceof CreateDropRoleStmt; }
public boolean isGrantRevokeRoleStmt() {
return stmt_ instanceof GrantRevokeRoleStmt;
@@ -171,7 +173,7 @@ public class AnalysisContext {
private boolean isViewMetadataStmt() {
return isShowFilesStmt() || isShowTablesStmt() || isShowDbsStmt() ||
- isShowFunctionsStmt() || isShowRolesStmt() || isShowGrantRoleStmt() ||
+ isShowFunctionsStmt() || isShowRolesStmt() || isShowGrantPrincipalStmt() ||
isShowCreateTableStmt() || isShowDataSrcsStmt() || isShowStatsStmt() ||
isDescribeTableStmt() || isDescribeDbStmt() || isShowCreateFunctionStmt();
}
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java b/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
new file mode 100644
index 0000000..6e84acc
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
@@ -0,0 +1,95 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.impala.analysis;
+
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import org.apache.impala.catalog.Principal;
+import org.apache.impala.common.AnalysisException;
+import org.apache.impala.common.InternalException;
+import org.apache.impala.thrift.TPrincipalType;
+import org.apache.impala.thrift.TShowGrantPrincipalParams;
+
+import java.util.List;
+
+/**
+ * Represents "SHOW GRANT ROLE <role>" [ON <privilegeSpec>]" and
+ * "SHOW GRANT USER <user> [ON <privilegeSpec>]" statements.
+ */
+public class ShowGrantPrincipalStmt extends AuthorizationStmt {
+ private final PrivilegeSpec privilegeSpec_;
+ private final String name_;
+ private final TPrincipalType principalType_;
+
+ // Set/modified during analysis.
+ private Principal principal_;
+
+ public ShowGrantPrincipalStmt(String name, TPrincipalType principalType,
+ PrivilegeSpec privilegeSpec) {
+ Preconditions.checkNotNull(name);
+ Preconditions.checkNotNull(principalType);
+ name_ = name;
+ principalType_ = principalType;
+ privilegeSpec_ = privilegeSpec;
+ }
+
+ @Override
+ public void analyze(Analyzer analyzer) throws AnalysisException {
+ super.analyze(analyzer);
+ if (Strings.isNullOrEmpty(name_)) {
+ throw new AnalysisException(String.format("%s name in SHOW GRANT %s cannot be " +
+ "empty.", Principal.toString(principalType_),
+ Principal.toString(principalType_).toUpperCase()));
+ }
+ principal_ = analyzer.getCatalog().getAuthPolicy().getPrincipal(name_,
+ principalType_);
+ if (principal_ == null) {
+ throw new AnalysisException(String.format("%s '%s' does not exist.",
+ Principal.toString(principalType_), name_));
+ }
+ if (privilegeSpec_ != null) privilegeSpec_.analyze(analyzer);
+ }
+
+ @Override
+ public String toSql() {
+ StringBuilder sb = new StringBuilder(String.format("SHOW GRANT %s ",
+ Principal.toString(principalType_).toUpperCase()));
+ sb.append(name_);
+ if (privilegeSpec_ != null) sb.append(" " + privilegeSpec_.toSql());
+ return sb.toString();
+ }
+
+ @Override
+ public void collectTableRefs(List<TableRef> tblRefs) {
+ if (privilegeSpec_ != null) privilegeSpec_.collectTableRefs(tblRefs);
+ }
+
+ public TShowGrantPrincipalParams toThrift() throws InternalException {
+ TShowGrantPrincipalParams params = new TShowGrantPrincipalParams();
+ params.setName(name_);
+ params.setPrincipal_type(principalType_);
+ params.setRequesting_user(requestingUser_.getShortName());
+ if (privilegeSpec_ != null) {
+ params.setPrivilege(privilegeSpec_.toThrift().get(0));
+ params.getPrivilege().setPrincipal_id(principal_.getId());
+ }
+ return params;
+ }
+
+ public Principal getPrincipal() { return principal_; }
+}
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java b/fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java
deleted file mode 100644
index 749bcc2..0000000
--- a/fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java
+++ /dev/null
@@ -1,86 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one
-// or more contributor license agreements. See the NOTICE file
-// distributed with this work for additional information
-// regarding copyright ownership. The ASF licenses this file
-// to you under the Apache License, Version 2.0 (the
-// "License"); you may not use this file except in compliance
-// with the License. You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing,
-// software distributed under the License is distributed on an
-// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-// KIND, either express or implied. See the License for the
-// specific language governing permissions and limitations
-// under the License.
-
-package org.apache.impala.analysis;
-
-import java.util.List;
-
-import org.apache.impala.catalog.Role;
-import org.apache.impala.common.AnalysisException;
-import org.apache.impala.common.InternalException;
-import org.apache.impala.thrift.TShowGrantRoleParams;
-
-import com.google.common.base.Preconditions;
-import com.google.common.base.Strings;
-
-/**
- * Represents a "SHOW GRANT ROLE <role> [ON <privilegeSpec>]" statement.
- */
-public class ShowGrantRoleStmt extends AuthorizationStmt {
- private final PrivilegeSpec privilegeSpec_;
- private final String roleName_;
-
- // Set/modified during analysis
- private Role role_;
-
- public ShowGrantRoleStmt(String roleName, PrivilegeSpec privilegeSpec) {
- Preconditions.checkNotNull(roleName);
- roleName_ = roleName;
- privilegeSpec_ = privilegeSpec;
- }
-
- public TShowGrantRoleParams toThrift() throws InternalException {
- TShowGrantRoleParams params = new TShowGrantRoleParams();
- params.setRole_name(roleName_);
- params.setRequesting_user(requestingUser_.getShortName());
- if (privilegeSpec_ != null) {
- params.setPrivilege(privilegeSpec_.toThrift().get(0));
- params.getPrivilege().setPrincipal_id(role_.getId());
- params.getPrivilege().setPrincipal_type(role_.getPrincipalType());
- }
- return params;
- }
-
- @Override
- public String toSql() {
- StringBuilder sb = new StringBuilder("SHOW GRANT ROLE ");
- sb.append(roleName_);
- if (privilegeSpec_ != null) sb.append(" " + privilegeSpec_.toSql());
- return sb.toString();
- }
-
- @Override
- public void collectTableRefs(List<TableRef> tblRefs) {
- if (privilegeSpec_ != null) privilegeSpec_.collectTableRefs(tblRefs);
- }
-
- @Override
- public void analyze(Analyzer analyzer) throws AnalysisException {
- super.analyze(analyzer);
- if (Strings.isNullOrEmpty(roleName_)) {
- throw new AnalysisException("Role name in SHOW GRANT ROLE cannot be " +
- "empty.");
- }
- role_ = analyzer.getCatalog().getAuthPolicy().getRole(roleName_);
- if (role_ == null) {
- throw new AnalysisException(String.format("Role '%s' does not exist.", roleName_));
- }
- if (privilegeSpec_ != null) privilegeSpec_.analyze(analyzer);
- }
-
- public Role getRole() { return role_; }
-}
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java b/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
index a151eb4..ffb8cfd 100644
--- a/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
+++ b/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java
@@ -413,30 +413,12 @@ public class AuthorizationPolicy implements PrivilegeCache {
}
/**
- * Returns the privileges that have been granted to a role as a tabular result set.
- * Allows for filtering based on a specific privilege spec or showing all privileges
- * granted to the role. Used by the SHOW GRANT ROLE statement.
- */
- public synchronized TResultSet getRolePrivileges(String roleName, TPrivilege filter) {
- return getPrincipalPrivileges(roleName, filter, TPrincipalType.ROLE);
- }
-
- /**
- * Returns the privileges that have been granted to a user as a tabular result set.
- * Allows for filtering based on a specific privilege spec or showing all privileges
- * granted to the user. Used by the SHOW GRANT USER statement.
- */
- public synchronized TResultSet getUserPrivileges(String userName, TPrivilege filter) {
- return getPrincipalPrivileges(userName, filter, TPrincipalType.USER);
- }
-
- /**
* Returns the privileges that have been granted to a principal as a tabular result set.
* Allows for filtering based on a specific privilege spec or showing all privileges
- * granted to the principal.
+ * granted to the principal. Used by the SHOW GRANT ROLE/USER statement.
*/
- private TResultSet getPrincipalPrivileges(String principalName, TPrivilege filter,
- TPrincipalType type) {
+ public synchronized TResultSet getPrincipalPrivileges(String principalName,
+ TPrivilege filter, TPrincipalType type) {
TResultSet result = new TResultSet();
result.setSchema(new TResultSetMetadata());
result.getSchema().addToColumns(new TColumn("scope", Type.STRING.toThrift()));
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/java/org/apache/impala/service/Frontend.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/Frontend.java b/fe/src/main/java/org/apache/impala/service/Frontend.java
index 29d917e..fcb5ce2 100644
--- a/fe/src/main/java/org/apache/impala/service/Frontend.java
+++ b/fe/src/main/java/org/apache/impala/service/Frontend.java
@@ -55,7 +55,7 @@ import org.apache.impala.analysis.InsertStmt;
import org.apache.impala.analysis.QueryStmt;
import org.apache.impala.analysis.ResetMetadataStmt;
import org.apache.impala.analysis.ShowFunctionsStmt;
-import org.apache.impala.analysis.ShowGrantRoleStmt;
+import org.apache.impala.analysis.ShowGrantPrincipalStmt;
import org.apache.impala.analysis.ShowRolesStmt;
import org.apache.impala.analysis.SqlParser;
import org.apache.impala.analysis.SqlScanner;
@@ -472,16 +472,17 @@ public class Frontend {
}
metadata.setColumns(Arrays.asList(
new TColumn("role_name", Type.STRING.toThrift())));
- } else if (analysis.isShowGrantRoleStmt()) {
- ddl.op_type = TCatalogOpType.SHOW_GRANT_ROLE;
- ShowGrantRoleStmt showGrantRoleStmt = (ShowGrantRoleStmt) analysis.getStmt();
- ddl.setShow_grant_role_params(showGrantRoleStmt.toThrift());
+ } else if (analysis.isShowGrantPrincipalStmt()) {
+ ddl.op_type = TCatalogOpType.SHOW_GRANT_PRINCIPAL;
+ ShowGrantPrincipalStmt showGrantPrincipalStmt =
+ (ShowGrantPrincipalStmt) analysis.getStmt();
+ ddl.setShow_grant_principal_params(showGrantPrincipalStmt.toThrift());
Set<String> groupNames =
getAuthzChecker().getUserGroups(analysis.getAnalyzer().getUser());
// User must be an admin to execute this operation if they have not been granted
- // this role.
- ddl.getShow_grant_role_params().setIs_admin_op(Sets.intersection(groupNames,
- showGrantRoleStmt.getRole().getGrantGroups()).isEmpty());
+ // this principal.
+ ddl.getShow_grant_principal_params().setIs_admin_op(Sets.intersection(groupNames,
+ showGrantPrincipalStmt.getPrincipal().getGrantGroups()).isEmpty());
metadata.setColumns(Arrays.asList(
new TColumn("name", Type.STRING.toThrift())));
} else if (analysis.isCreateDropRoleStmt()) {
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/main/java/org/apache/impala/service/JniFrontend.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/JniFrontend.java b/fe/src/main/java/org/apache/impala/service/JniFrontend.java
index ce16b51..2781865 100644
--- a/fe/src/main/java/org/apache/impala/service/JniFrontend.java
+++ b/fe/src/main/java/org/apache/impala/service/JniFrontend.java
@@ -87,7 +87,7 @@ import org.apache.impala.thrift.TMetadataOpRequest;
import org.apache.impala.thrift.TQueryCtx;
import org.apache.impala.thrift.TResultSet;
import org.apache.impala.thrift.TShowFilesParams;
-import org.apache.impala.thrift.TShowGrantRoleParams;
+import org.apache.impala.thrift.TShowGrantPrincipalParams;
import org.apache.impala.thrift.TShowRolesParams;
import org.apache.impala.thrift.TShowRolesResult;
import org.apache.impala.thrift.TShowStatsOp;
@@ -551,11 +551,16 @@ public class JniFrontend {
}
}
- public byte[] getRolePrivileges(byte[] showGrantRolesParams) throws ImpalaException {
- TShowGrantRoleParams params = new TShowGrantRoleParams();
- JniUtil.deserializeThrift(protocolFactory_, params, showGrantRolesParams);
- TResultSet result = frontend_.getCatalog().getAuthPolicy().getRolePrivileges(
- params.getRole_name(), params.getPrivilege());
+ /**
+ * Gets the principal privileges for the given principal.
+ */
+ public byte[] getPrincipalPrivileges(byte[] showGrantPrincipalParams)
+ throws ImpalaException {
+ TShowGrantPrincipalParams params = new TShowGrantPrincipalParams();
+ JniUtil.deserializeThrift(protocolFactory_, params, showGrantPrincipalParams);
+ TResultSet result;
+ result = frontend_.getCatalog().getAuthPolicy().getPrincipalPrivileges(
+ params.getName(), params.getPrivilege(), params.getPrincipal_type());
TSerializer serializer = new TSerializer(protocolFactory_);
try {
return serializer.serialize(result);
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
index dd58131..580434a 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
@@ -22,6 +22,7 @@ import java.util.HashSet;
import org.apache.impala.authorization.AuthorizationConfig;
import org.apache.impala.catalog.Catalog;
import org.apache.impala.catalog.Role;
+import org.apache.impala.catalog.User;
import org.apache.impala.common.AnalysisException;
import org.apache.impala.testutil.TestUtils;
import org.apache.impala.thrift.TQueryCtx;
@@ -32,6 +33,8 @@ public class AnalyzeAuthStmtsTest extends AnalyzerTest {
public AnalyzeAuthStmtsTest() {
catalog_.getAuthPolicy().addPrincipal(
new Role("myRole", new HashSet<String>()));
+ catalog_.getAuthPolicy().addPrincipal(
+ new User("myUser", new HashSet<String>()));
}
@Override
@@ -70,22 +73,30 @@ public class AnalyzeAuthStmtsTest extends AnalyzerTest {
}
@Test
- public void AnalyzeShowGrantRole() {
- AnalyzesOk("SHOW GRANT ROLE myRole");
- AnalyzesOk("SHOW GRANT ROLE myRole ON SERVER");
- AnalyzesOk("SHOW GRANT ROLE myRole ON DATABASE functional");
- AnalyzesOk("SHOW GRANT ROLE myRole ON TABLE functional.alltypes");
- AnalyzesOk("SHOW GRANT ROLE myRole ON URI 'hdfs:////test-warehouse//foo'");
+ public void AnalyzeShowGrantPrincipal() {
+ for (String type: new String[]{"ROLE myRole", "USER myUser"}) {
+ AnalyzesOk(String.format("SHOW GRANT %s", type));
+ AnalyzesOk(String.format("SHOW GRANT %s ON SERVER", type));
+ AnalyzesOk(String.format("SHOW GRANT %s ON DATABASE functional", type));
+ AnalyzesOk(String.format("SHOW GRANT %s ON TABLE functional.alltypes", type));
+ AnalyzesOk(String.format("SHOW GRANT %s ON URI 'hdfs:////test-warehouse//foo'",
+ type));
+
+ AnalysisContext authDisabledCtx = createAuthDisabledAnalysisCtx();
+ AnalysisError("SHOW GRANT ROLE myRole", authDisabledCtx,
+ "Authorization is not enabled.");
+ AnalysisError("SHOW GRANT ROLE myRole ON SERVER", authDisabledCtx,
+ "Authorization is not enabled.");
+ }
AnalysisError("SHOW GRANT ROLE does_not_exist",
"Role 'does_not_exist' does not exist.");
AnalysisError("SHOW GRANT ROLE does_not_exist ON SERVER",
"Role 'does_not_exist' does not exist.");
- AnalysisContext authDisabledCtx = createAuthDisabledAnalysisCtx();
- AnalysisError("SHOW GRANT ROLE myRole", authDisabledCtx,
- "Authorization is not enabled.");
- AnalysisError("SHOW GRANT ROLE myRole ON SERVER", authDisabledCtx,
- "Authorization is not enabled.");
+ AnalysisError("SHOW GRANT USER does_not_exist",
+ "User 'does_not_exist' does not exist.");
+ AnalysisError("SHOW GRANT USER does_not_exist ON SERVER",
+ "User 'does_not_exist' does not exist.");
}
@Test
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
index 7e10eee..a750c6f 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
@@ -1093,16 +1093,20 @@ public class AuthorizationStmtTest extends FrontendTestBase {
// Show role grant group should always be allowed.
authorize(String.format("show role grant group `%s`", USER.getName())).ok();
- // Show grant role should always be allowed.
+ // Show grant role/user should always be allowed.
try {
authzCatalog_.addRole("test_role");
- authorize("show grant role test_role").ok();
- authorize("show grant role test_role on server").ok();
- authorize("show grant role test_role on database functional").ok();
- authorize("show grant role test_role on table functional.alltypes").ok();
- authorize("show grant role test_role on uri '/test-warehouse'").ok();
+ authzCatalog_.addUser("test_user");
+ for (String type : new String[]{"role test_role", "user test_user"}) {
+ authorize(String.format("show grant %s", type)).ok();
+ authorize(String.format("show grant %s on server", type)).ok();
+ authorize(String.format("show grant %s on database functional", type)).ok();
+ authorize(String.format("show grant %s on table functional.alltypes", type)).ok();
+ authorize(String.format("show grant %s on uri '/test-warehouse'", type)).ok();
+ }
} finally {
authzCatalog_.removeRole("test_role");
+ authzCatalog_.removeUser("test_user");
}
// Show create table.
http://git-wip-us.apache.org/repos/asf/impala/blob/73d37d6e/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
index a86f467..be97d67 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
@@ -3675,22 +3675,25 @@ public class ParserTest extends FrontendTestBase {
}
@Test
- public void TestShowGrantRole() {
- // Show all grants on a role
- ParsesOk("SHOW GRANT ROLE foo");
-
- // Show grants on a specific object
- ParsesOk("SHOW GRANT ROLE foo ON SERVER");
- ParsesOk("SHOW GRANT ROLE foo ON DATABASE foo");
- ParsesOk("SHOW GRANT ROLE foo ON TABLE foo");
- ParsesOk("SHOW GRANT ROLE foo ON TABLE foo.bar");
- ParsesOk("SHOW GRANT ROLE foo ON URI '/abc/123'");
-
- ParserError("SHOW GRANT ROLE");
- ParserError("SHOW GRANT ROLE foo ON SERVER foo");
- ParserError("SHOW GRANT ROLE foo ON DATABASE");
- ParserError("SHOW GRANT ROLE foo ON TABLE");
- ParserError("SHOW GRANT ROLE foo ON URI abc");
+ public void TestShowGrantPrincipal() {
+ for (String type: new String[]{"ROLE", "USER"}) {
+ // Show all grants on a particular principal type.
+ ParsesOk(String.format("SHOW GRANT %s foo", type));
+
+ // Show grants on a specific object
+ ParsesOk(String.format("SHOW GRANT %s foo ON SERVER", type));
+ ParsesOk(String.format("SHOW GRANT %s foo ON DATABASE foo", type));
+ ParsesOk(String.format("SHOW GRANT %s foo ON TABLE foo", type));
+ ParsesOk(String.format("SHOW GRANT %s foo ON TABLE foo.bar", type));
+ ParsesOk(String.format("SHOW GRANT %s foo ON URI '/abc/123'", type));
+
+ ParserError(String.format("SHOW GRANT %s", type));
+ ParserError(String.format("SHOW GRANT %s foo ON SERVER foo", type));
+ ParserError(String.format("SHOW GRANT %s foo ON DATABASE", type));
+ ParserError(String.format("SHOW GRANT %s foo ON TABLE", type));
+ ParserError(String.format("SHOW GRANT %s foo ON URI abc", type));
+ }
+ ParserError("SHOW GRANT FOO bar");
}
@Test
[2/4] impala git commit: tests: remove unused failure_injector test
library
Posted by tm...@apache.org.
tests: remove unused failure_injector test library
This seems to have been added years ago but never used. It's just
clutter right now that distracts from newer failure injection
approaches. Let's remove it.
Change-Id: I6ae8aa13acc85ec2f3a345d460babdb8c55a56cd
Reviewed-on: http://gerrit.cloudera.org:8080/11226
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/f8f200e5
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/f8f200e5
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/f8f200e5
Branch: refs/heads/master
Commit: f8f200e58ddfb37abac9fedaad7ee4b5516b244f
Parents: 73d37d6
Author: Todd Lipcon <to...@apache.org>
Authored: Tue Aug 14 16:46:37 2018 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Mon Aug 20 18:15:57 2018 +0000
----------------------------------------------------------------------
tests/common/failure_injector.py | 102 ----------------------------------
1 file changed, 102 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/f8f200e5/tests/common/failure_injector.py
----------------------------------------------------------------------
diff --git a/tests/common/failure_injector.py b/tests/common/failure_injector.py
deleted file mode 100644
index 95a8393..0000000
--- a/tests/common/failure_injector.py
+++ /dev/null
@@ -1,102 +0,0 @@
-# Licensed to the Apache Software Foundation (ASF) under one
-# or more contributor license agreements. See the NOTICE file
-# distributed with this work for additional information
-# regarding copyright ownership. The ASF licenses this file
-# to you under the Apache License, Version 2.0 (the
-# "License"); you may not use this file except in compliance
-# with the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing,
-# software distributed under the License is distributed on an
-# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
-# KIND, either express or implied. See the License for the
-# specific language governing permissions and limitations
-# under the License.
-
-# Failure injection module for the Impala service. There are two main ways this module
-# can be used - the first is to initialize the failure injector and then call start()
-# which will kick off a timer that chooses a random impalad/state store process
-# to fail each time timer fires.
-# The second way this module can be used to to initialize it and call the actions
-# directly (ex. kill_random_impalad()). This provides a bit more control over exactly
-# when a failure will happen and is useful for targeted test scenarios.
-
-import logging
-from random import choice
-from threading import Timer
-
-logging.basicConfig(level=logging.INFO, format='%(threadName)s: %(message)s')
-LOG = logging.getLogger('failure-injector')
-
-# This class is used for injecting failures for the Impala service.
-class FailureInjector(object):
- def __init__(self, impala_cluster, failure_frequency, impalad_exclude_list=None):
- """
- Initializes the FailureInjector object.
-
- impala_cluster - An ImpalaCluster object (see the impala_cluster module)
- failure_frequency - Interval to fire timer (in seconds)
- impalad_exclude_list - A list of impalad host:port name to not inject failures
- on. Useful to filter out the coordinator.
- """
- self.cluster = impala_cluster
- self.cluster.get_impala_service().set_process_auto_restart_config(value=True)
- # TODO: Do we need to restart the impala service to apply this?
- # self.cluster.get_impala_service().restart()
- self.failure_frequency = failure_frequency
- num_impalad_procs = len(self.cluster.get_impala_service().get_all_impalad_processes())
-
- self.impalad_exclude_list = impalad_exclude_list
-
- # Build a weighted list of possible actions. This is done using a trivial approach
- # where we just add the item multiple times (weight value) into the action list.
- # TODO: Provide a way to dynamically configure the weights
- actions_with_weights = {self.kill_random_impalad: num_impalad_procs * 2,
- self.kill_state_store: 1}
-
- self.possible_actions = list()
- for key, value in actions_with_weights.items():
- self.possible_actions.extend([key] * value)
-
- def start(self):
- """ Starts the timer, triggering failures for the specified interval """
- self.__start_timer()
-
- def cancel(self):
- """ Stops the timer, canceling any additional failures from occurring """
- if self.__timer is not None:
- self.__timer.cancel()
-
- def kill_random_impalad(self):
- """ Kills a randomly selected impalad instance not in the exlude list """
- filtered_impalad = \
- filter(lambda impalad: '%s:%d' % (impalad.hostname, impalad.be_port)\
- not in self.impalad_exclude_list,
- self.cluster.get_impala_service().get_all_impalad_processes())
- self.kill_impalad(choice(filtered_impalad))
-
- def kill_impalad(self, impalad):
- """ Kills the specified impalad instance """
- LOG.info('Chose impalad on "%s" to kill' % impalad.hostname)
- impalad.kill()
-
- def kill_state_store(self):
- """ Kills the statestore process """
- state_store = self.cluster.get_impala_service().get_state_store_process()
- LOG.info('Chose statestore on "%s" to kill' % state_store.hostname)
- state_store.kill()
-
- def __start_timer(self):
- """ Starts a new timer, cancelling the previous timer if it is running """
- self.cancel()
- self.__timer = Timer(self.failure_frequency, self.__choose_action)
- self.__timer.start()
-
- def __choose_action(self):
- """ Chooses a failure action to perform """
- action = choice(self.possible_actions)
- LOG.info('Executing action: %s' % action)
- action()
- self.__start_timer()
[3/4] impala git commit: IMPALA-7407: Fix test_cancellation failure
on KeyboardInterrupt
Posted by tm...@apache.org.
IMPALA-7407: Fix test_cancellation failure on KeyboardInterrupt
test_cancellation runs a shell process, executes a query, sleeps,
sends a sigint to the process, and then checks that the query is
cancelled. If the sigint is sent prior to the shell installing its
signal handler, the test can fail with a KeyboardInterrupt.
This patch removes the reliance on the sleep being long enough by
actually reading the output of the shell and only cancelling the
query once the shell shows that it has started running.
Testing:
- Ran test_cancellation in a loop.
Change-Id: I65302ffb838d5185f77853bc2e53296f3a701d93
Reviewed-on: http://gerrit.cloudera.org:8080/11255
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Thomas Marshall <th...@cmu.edu>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/dccc2de8
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/dccc2de8
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/dccc2de8
Branch: refs/heads/master
Commit: dccc2de86a01e1a109f215c1a08d118471f57ca4
Parents: f8f200e
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
Authored: Tue Aug 14 20:43:52 2018 +0000
Committer: Thomas Marshall <th...@cmu.edu>
Committed: Mon Aug 20 19:56:11 2018 +0000
----------------------------------------------------------------------
tests/shell/test_shell_commandline.py | 2 +-
tests/shell/util.py | 16 ++++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/dccc2de8/tests/shell/test_shell_commandline.py
----------------------------------------------------------------------
diff --git a/tests/shell/test_shell_commandline.py b/tests/shell/test_shell_commandline.py
index b0f2f03..0f73620 100644
--- a/tests/shell/test_shell_commandline.py
+++ b/tests/shell/test_shell_commandline.py
@@ -340,7 +340,7 @@ class TestImpalaShell(ImpalaTestSuite):
query = "set num_nodes=1; set mt_dop=1; set batch_size=1; \
select sleep(10) from functional_parquet.alltypesagg"
p = ImpalaShell('-q "{0}"'.format(query))
- sleep(6)
+ p.wait_for_query_start()
os.kill(p.pid(), signal.SIGINT)
result = p.get_result()
http://git-wip-us.apache.org/repos/asf/impala/blob/dccc2de8/tests/shell/util.py
----------------------------------------------------------------------
diff --git a/tests/shell/util.py b/tests/shell/util.py
index 0c899ab..6c1df0e 100755
--- a/tests/shell/util.py
+++ b/tests/shell/util.py
@@ -136,6 +136,22 @@ class ImpalaShell(object):
# Allow fluent-style chaining of commands
return self
+ def wait_for_query_start(self):
+ """If this shell was started with the '-q' option, this mathod will block until the
+ query has started running"""
+ # readline() will block until a line is actually printed, so this loop should always
+ # read something like:
+ # Starting Impala Shell without Kerberos authentication
+ # Connected to localhost:21000
+ # Server version: impalad version...
+ # Query: select sleep(10)
+ # Query submitted at:...
+ # Query progress can be monitored at:...
+ # We stop at 10 iterations to prevent an infinite loop if somehting goes wrong.
+ iters = 0
+ while "Query progress" not in self.shell_process.stderr.readline() and iters < 10:
+ iters += 1
+
def get_result(self, stdin_input=None):
"""Returns an ImpalaShellResult produced by the shell process on exit. After this
method returns, send_cmd() no longer has any effect."""
[4/4] impala git commit: IMPALA-7453. Intern HdfsStorageDescriptors
Posted by tm...@apache.org.
IMPALA-7453. Intern HdfsStorageDescriptors
The number of unique HdfsStorageDescriptors in a warehouse is typically
much smaller than the number of partitions. Each object takes 32/40 bytes
(with/without compressed OOPs respectively). So, by interning these
objects, we can save that amount of memory as well as one object per
partition.
The overall savings aren't huge (on the order of tens of MBs) but the
change is pretty simple so seems worthwhile.
This patch also pulls in the errorprone annotations into the pom so that
errorprone can ensure that the class can be annotated as Immutable.
errorprone checks that classes annotated as Immutable only contain
immutable fields.
I tested this change by comparing 'jmap -histo:live' on a catalogd
before/after. For my local dev environment test warehouse, I had 12055
instances (385kb) before the change and 24 instances (768 bytes) after.
Change-Id: I9ef93148d629b060fa9f67c631e9c3d904a0ccf9
Reviewed-on: http://gerrit.cloudera.org:8080/11236
Reviewed-by: Bharath Vissapragada <bh...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/d2930028
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/d2930028
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/d2930028
Branch: refs/heads/master
Commit: d29300281b5d07c1ed98032536c008b076a7baa5
Parents: dccc2de
Author: Todd Lipcon <to...@apache.org>
Authored: Tue Aug 7 19:01:43 2018 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Mon Aug 20 20:32:42 2018 +0000
----------------------------------------------------------------------
fe/pom.xml | 6 ++
.../apache/impala/catalog/HdfsPartition.java | 16 ++---
.../impala/catalog/HdfsStorageDescriptor.java | 71 +++++++++++++++++---
.../impala/catalog/local/LocalKuduTable.java | 3 +-
.../catalog/local/LocalPartitionSpec.java | 6 +-
.../org/apache/impala/catalog/CatalogTest.java | 10 +++
6 files changed, 86 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/d2930028/fe/pom.xml
----------------------------------------------------------------------
diff --git a/fe/pom.xml b/fe/pom.xml
index a120536..c695a5b 100644
--- a/fe/pom.xml
+++ b/fe/pom.xml
@@ -454,6 +454,12 @@ under the License.
</dependency>
<dependency>
+ <groupId>com.google.errorprone</groupId>
+ <artifactId>error_prone_annotations</artifactId>
+ <version>2.3.1</version>
+ </dependency>
+
+ <dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junit.version}</version>
http://git-wip-us.apache.org/repos/asf/impala/blob/d2930028/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java b/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
index 05b66f7..14ce4e3 100644
--- a/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
+++ b/fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
@@ -459,7 +459,7 @@ public class HdfsPartition implements FeFsPartition, PrunablePartition {
* we. We should therefore treat mixing formats inside one partition as user error.
* It's easy to add per-file metadata to FileDescriptor if this changes.
*/
- private final HdfsStorageDescriptor fileFormatDescriptor_;
+ private HdfsStorageDescriptor fileFormatDescriptor_;
/**
* The file descriptors of this partition, encoded as flatbuffers. Storing the raw
@@ -563,7 +563,8 @@ public class HdfsPartition implements FeFsPartition, PrunablePartition {
* format classes.
*/
public void setFileFormat(HdfsFileFormat fileFormat) {
- fileFormatDescriptor_.setFileFormat(fileFormat);
+ fileFormatDescriptor_ = fileFormatDescriptor_.cloneWithChangedFileFormat(
+ fileFormat);
cachedMsPartitionDescriptor_.sdInputFormat = fileFormat.inputFormat();
cachedMsPartitionDescriptor_.sdOutputFormat = fileFormat.outputFormat();
cachedMsPartitionDescriptor_.sdSerdeInfo.setSerializationLib(
@@ -811,15 +812,8 @@ public class HdfsPartition implements FeFsPartition, PrunablePartition {
public static HdfsPartition fromThrift(HdfsTable table,
long id, THdfsPartition thriftPartition) {
- HdfsStorageDescriptor storageDesc = new HdfsStorageDescriptor(table.getName(),
- HdfsFileFormat.fromThrift(thriftPartition.getFileFormat()),
- thriftPartition.lineDelim,
- thriftPartition.fieldDelim,
- thriftPartition.collectionDelim,
- thriftPartition.mapKeyDelim,
- thriftPartition.escapeChar,
- (byte) '"', // TODO: We should probably add quoteChar to THdfsPartition.
- thriftPartition.blockSize);
+ HdfsStorageDescriptor storageDesc = HdfsStorageDescriptor.fromThriftPartition(
+ thriftPartition, table.getName());
List<LiteralExpr> literalExpr = Lists.newArrayList();
if (id != CatalogObjectsConstants.PROTOTYPE_PARTITION_ID) {
http://git-wip-us.apache.org/repos/asf/impala/blob/d2930028/fe/src/main/java/org/apache/impala/catalog/HdfsStorageDescriptor.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/HdfsStorageDescriptor.java b/fe/src/main/java/org/apache/impala/catalog/HdfsStorageDescriptor.java
index f51b10e..d1c0f9d 100644
--- a/fe/src/main/java/org/apache/impala/catalog/HdfsStorageDescriptor.java
+++ b/fe/src/main/java/org/apache/impala/catalog/HdfsStorageDescriptor.java
@@ -17,22 +17,32 @@
package org.apache.impala.catalog;
-import java.util.List;
import java.util.Map;
+import java.util.Objects;
import org.apache.hadoop.hive.metastore.api.SerDeInfo;
import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
import org.apache.hadoop.hive.serde.serdeConstants;
+import org.apache.impala.thrift.THdfsPartition;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Interner;
+import com.google.common.collect.Interners;
import com.google.common.collect.Maps;
+import com.google.errorprone.annotations.Immutable;
/**
* Represents the file format metadata for files stored in a table or partition.
+ *
+ * This class is immutable, and instances are stored in a global interner, since
+ * the number of distinct combinations of file formats and other parameters
+ * is typically quite low relative to the total number of partitions (e.g. almost
+ * all partitions will use the default quoting).
*/
+@Immutable
public class HdfsStorageDescriptor {
public static final char DEFAULT_LINE_DELIM = '\n';
// hive by default uses ctrl-a as field delim
@@ -46,14 +56,14 @@ public class HdfsStorageDescriptor {
// Important: don't change the ordering of these keys - if e.g. FIELD_DELIM is not
// found, the value of LINE_DELIM is used, so LINE_DELIM must be found first.
// Package visible for testing.
- final static List<String> DELIMITER_KEYS = ImmutableList.of(
+ final static ImmutableList<String> DELIMITER_KEYS = ImmutableList.of(
serdeConstants.LINE_DELIM, serdeConstants.FIELD_DELIM,
serdeConstants.COLLECTION_DELIM, serdeConstants.MAPKEY_DELIM,
serdeConstants.ESCAPE_CHAR, serdeConstants.QUOTE_CHAR);
// The Parquet serde shows up multiple times as the location of the implementation
// has changed between Impala versions.
- final static List<String> COMPATIBLE_SERDES = ImmutableList.of(
+ final static ImmutableList<String> COMPATIBLE_SERDES = ImmutableList.of(
"org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe", // (seq / text / parquet)
"org.apache.hadoop.hive.serde2.avro.AvroSerDe", // (avro)
"org.apache.hadoop.hive.serde2.columnar.ColumnarSerDe", // (rc)
@@ -65,7 +75,10 @@ public class HdfsStorageDescriptor {
private final static Logger LOG = LoggerFactory.getLogger(HdfsStorageDescriptor.class);
- private HdfsFileFormat fileFormat_;
+ private final static Interner<HdfsStorageDescriptor> INTERNER =
+ Interners.newWeakInterner();
+
+ private final HdfsFileFormat fileFormat_;
private final byte lineDelim_;
private final byte fieldDelim_;
private final byte collectionDelim_;
@@ -74,10 +87,6 @@ public class HdfsStorageDescriptor {
private final byte quoteChar_;
private final int blockSize_;
- public void setFileFormat(HdfsFileFormat fileFormat) {
- fileFormat_ = fileFormat;
- }
-
/**
* Returns a map from delimiter key to a single delimiter character,
* filling in defaults if explicit values are not found in the supplied
@@ -151,7 +160,7 @@ public class HdfsStorageDescriptor {
return null;
}
- public HdfsStorageDescriptor(String tblName, HdfsFileFormat fileFormat, byte lineDelim,
+ private HdfsStorageDescriptor(String tblName, HdfsFileFormat fileFormat, byte lineDelim,
byte fieldDelim, byte collectionDelim, byte mapKeyDelim, byte escapeChar,
byte quoteChar, int blockSize) {
this.fileFormat_ = fileFormat;
@@ -216,7 +225,7 @@ public class HdfsStorageDescriptor {
}
try {
- return new HdfsStorageDescriptor(tblName,
+ return INTERNER.intern(new HdfsStorageDescriptor(tblName,
HdfsFileFormat.fromJavaClassName(sd.getInputFormat()),
delimMap.get(serdeConstants.LINE_DELIM),
delimMap.get(serdeConstants.FIELD_DELIM),
@@ -224,13 +233,30 @@ public class HdfsStorageDescriptor {
delimMap.get(serdeConstants.MAPKEY_DELIM),
delimMap.get(serdeConstants.ESCAPE_CHAR),
delimMap.get(serdeConstants.QUOTE_CHAR),
- blockSize);
+ blockSize));
} catch (IllegalArgumentException ex) {
// Thrown by fromJavaClassName
throw new InvalidStorageDescriptorException(ex);
}
}
+ public static HdfsStorageDescriptor fromThriftPartition(THdfsPartition thriftPartition,
+ String tableName) {
+ return INTERNER.intern(new HdfsStorageDescriptor(tableName,
+ HdfsFileFormat.fromThrift(thriftPartition.getFileFormat()),
+ thriftPartition.lineDelim, thriftPartition.fieldDelim,
+ thriftPartition.collectionDelim, thriftPartition.mapKeyDelim,
+ thriftPartition.escapeChar,
+ (byte) '"', // TODO: We should probably add quoteChar to THdfsPartition.
+ thriftPartition.blockSize));
+ }
+
+ public HdfsStorageDescriptor cloneWithChangedFileFormat(HdfsFileFormat newFormat) {
+ return INTERNER.intern(new HdfsStorageDescriptor(
+ "<unknown>", newFormat, lineDelim_, fieldDelim_, collectionDelim_, mapKeyDelim_,
+ escapeChar_, quoteChar_, blockSize_));
+ }
+
public byte getLineDelim() { return lineDelim_; }
public byte getFieldDelim() { return fieldDelim_; }
public byte getCollectionDelim() { return collectionDelim_; }
@@ -238,4 +264,27 @@ public class HdfsStorageDescriptor {
public byte getEscapeChar() { return escapeChar_; }
public HdfsFileFormat getFileFormat() { return fileFormat_; }
public int getBlockSize() { return blockSize_; }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(blockSize_, collectionDelim_, escapeChar_, fieldDelim_,
+ fileFormat_, lineDelim_, mapKeyDelim_, quoteChar_);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) return true;
+ if (obj == null) return false;
+ if (getClass() != obj.getClass()) return false;
+ HdfsStorageDescriptor other = (HdfsStorageDescriptor) obj;
+ if (blockSize_ != other.blockSize_) return false;
+ if (collectionDelim_ != other.collectionDelim_) return false;
+ if (escapeChar_ != other.escapeChar_) return false;
+ if (fieldDelim_ != other.fieldDelim_) return false;
+ if (fileFormat_ != other.fileFormat_) return false;
+ if (lineDelim_ != other.lineDelim_) return false;
+ if (mapKeyDelim_ != other.mapKeyDelim_) return false;
+ if (quoteChar_ != other.quoteChar_) return false;
+ return true;
+ }
}
http://git-wip-us.apache.org/repos/asf/impala/blob/d2930028/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
index 8a6bb67..2095449 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalKuduTable.java
@@ -22,8 +22,6 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
-import javax.annotation.concurrent.Immutable;
-
import org.apache.hadoop.hive.metastore.api.FieldSchema;
import org.apache.hadoop.hive.metastore.api.Table;
import org.apache.impala.analysis.ColumnDef;
@@ -46,6 +44,7 @@ import org.apache.kudu.client.KuduException;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
+import com.google.errorprone.annotations.Immutable;
public class LocalKuduTable extends LocalTable implements FeKuduTable {
private final TableParams tableParams_;
http://git-wip-us.apache.org/repos/asf/impala/blob/d2930028/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java
index f48f54e..690a4bf 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalPartitionSpec.java
@@ -19,8 +19,6 @@ package org.apache.impala.catalog.local;
import java.util.List;
-import javax.annotation.concurrent.Immutable;
-
import org.apache.hadoop.hive.metastore.api.MetaException;
import org.apache.impala.analysis.LiteralExpr;
import org.apache.impala.catalog.CatalogException;
@@ -30,6 +28,7 @@ import org.apache.impala.util.MetaStoreUtil;
import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
+import com.google.errorprone.annotations.Immutable;
/**
* Specification of a partition of a {@link LocalFsTable} containing
@@ -39,6 +38,9 @@ import com.google.common.collect.ImmutableList;
class LocalPartitionSpec implements PrunablePartition {
private final long id_;
private final String name_;
+
+ // LiteralExprs are technically mutable prior to analysis.
+ @SuppressWarnings("Immutable")
private final ImmutableList<LiteralExpr> partitionValues_;
LocalPartitionSpec(LocalFsTable table, String partName, long id) {
http://git-wip-us.apache.org/repos/asf/impala/blob/d2930028/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java b/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
index ad1595b..bc90820 100644
--- a/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
@@ -27,8 +27,10 @@ import static org.junit.Assert.assertTrue;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Set;
@@ -359,6 +361,8 @@ public class CatalogTest {
// check that partition keys cover the date range 1/1/2009-12/31/2010
// and that we have one file per partition.
assertEquals(24, partitions.size());
+ Set<HdfsStorageDescriptor> uniqueSds = Collections.newSetFromMap(
+ new IdentityHashMap<HdfsStorageDescriptor, Boolean>());
Set<Long> months = Sets.newHashSet();
for (FeFsPartition p: partitions) {
assertEquals(2, p.getPartitionValues().size());
@@ -380,8 +384,14 @@ public class CatalogTest {
// no need for this boolean anymore.
assertEquals(p.getFileDescriptors().size(), 1);
}
+
+ uniqueSds.add(p.getInputFormatDescriptor());
}
assertEquals(months.size(), 24);
+
+ // We intern storage descriptors, so we should only have a single instance across
+ // all of the partitions.
+ assertEquals(1, uniqueSds.size());
}
// TODO: All Hive-stats related tests are temporarily disabled because of an unknown,