You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2019/06/13 06:37:14 UTC
[impala] branch master updated (ab908d5 -> 94652d7)
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git.
from ab908d5 IMPALA-8605: clean up HS2/beeswax session management
new 25c8b62 Update mustache to commit b290952d8eb93d085214d8c8c9eab8559df9f606
new fcaec27 IMPALA-8436: Prohibit write/alter operations on materialized view
new d9f17c9 IMPALA-8649: Fix confusing SHOW GRANT error messages
new 5b0c098 IMPALA-8588: Fix revoke grant option with Ranger
new e3117aa IMPALA-7154: Increase metastore client time out for tests
new ff4e0a3 IMPALA-8629: (part 2) Adjust new KuduStorageHandler package
new 94652d7 IMPALA-8143: Enhance DoRpcWithRetry().
The 7 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:
be/src/rpc/impala-service-pool.cc | 13 +-
be/src/rpc/impala-service-pool.h | 3 +
be/src/rpc/rpc-mgr-test.cc | 87 ++++++-
be/src/rpc/rpc-mgr-test.h | 27 +-
be/src/rpc/rpc-mgr.h | 15 ++
be/src/rpc/rpc-mgr.inline.h | 36 +++
be/src/runtime/coordinator-backend-state.cc | 14 +-
be/src/runtime/query-state.h | 2 +-
be/src/service/client-request-state.cc | 13 +-
be/src/service/control-service.h | 23 --
be/src/thirdparty/mustache/README | 2 +-
be/src/thirdparty/mustache/mustache.cc | 280 +++++++++++++--------
be/src/thirdparty/mustache/mustache.h | 2 +-
be/src/util/webserver.cc | 4 +-
bin/impala-config.sh | 4 +-
.../org/apache/impala/compat/MetastoreShim.java | 24 ++
.../org/apache/impala/analysis/PrivilegeSpec.java | 14 +-
.../ranger/RangerCatalogdAuthorizationManager.java | 32 +--
.../org/apache/impala/catalog/HdfsFileFormat.java | 7 +-
.../java/org/apache/impala/catalog/KuduTable.java | 9 +-
.../main/java/org/apache/impala/catalog/Table.java | 4 +-
.../impala/analysis/AnalyzeAuthStmtsTest.java | 37 ++-
.../apache/impala/analysis/AnalyzeKuduDDLTest.java | 20 +-
.../org/apache/impala/analysis/AnalyzerTest.java | 19 ++
.../java/org/apache/impala/analysis/ToSqlTest.java | 5 +-
.../authorization/AuthorizationTestBase.java | 12 +-
.../org/apache/impala/planner/PlannerTestBase.java | 7 +-
fe/src/test/resources/hive-site.xml.py | 11 +-
.../functional/functional_schema_template.sql | 10 +
.../queries/PlannerTest/kudu.test | 28 +--
tests/authorization/test_ranger.py | 29 ++-
tests/custom_cluster/test_kudu.py | 10 +-
tests/query_test/test_kudu.py | 5 +-
33 files changed, 564 insertions(+), 244 deletions(-)
[impala] 03/07: IMPALA-8649: Fix confusing SHOW GRANT error messages
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit d9f17c963407bfcb19c89dcaad6e9be17522cd1e
Author: Fredy Wijaya <fw...@cloudera.com>
AuthorDate: Tue Jun 11 15:00:31 2019 -0700
IMPALA-8649: Fix confusing SHOW GRANT error messages
Before this patch, PrivilegeSpec had error messages tailored to
GRANT and REVOKE statements. However, PrivilegeSpec is also used in SHOW
GRANT statement. This patch updates the error messages to be less
confusing in SHOW GRANT statement.
Testing:
- Added new tests in AnalyzeAuthStmtsTest
- Ran AnalyzeAuthStmtsTest
Change-Id: Ibb88bdc19cd1223902b44e3634f756d086332266
Reviewed-on: http://gerrit.cloudera.org:8080/13587
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../org/apache/impala/analysis/PrivilegeSpec.java | 14 ++++----
.../impala/analysis/AnalyzeAuthStmtsTest.java | 37 +++++++++++++++-------
2 files changed, 33 insertions(+), 18 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java b/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
index e03c850..41a96a0 100644
--- a/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
+++ b/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
@@ -201,9 +201,9 @@ public class PrivilegeSpec extends StmtNode {
try {
analyzer.getDb(dbName_, true);
} catch (AnalysisException e) {
- throw new AnalysisException(String.format("Error setting privileges for " +
- "database '%s'. Verify that the database exists and that you have " +
- "permissions to issue a GRANT/REVOKE statement.", dbName_));
+ throw new AnalysisException(String.format("Error setting/showing privileges " +
+ "for database '%s'. Verify that the database exists and that you have " +
+ "permissions to issue a GRANT/REVOKE/SHOW GRANT statement.", dbName_));
}
break;
case URI:
@@ -252,9 +252,9 @@ public class PrivilegeSpec extends StmtNode {
for (String columnName: columnNames_) {
if (table.getColumn(columnName) == null) {
// The error message should not reveal the existence or absence of a column.
- throw new AnalysisException(String.format("Error setting column-level " +
+ throw new AnalysisException(String.format("Error setting/showing column-level " +
"privileges for table '%s'. Verify that both table and columns exist " +
- "and that you have permissions to issue a GRANT/REVOKE statement.",
+ "and that you have permissions to issue a GRANT/REVOKE/SHOW GRANT statement.",
tableName_.toString()));
}
}
@@ -284,9 +284,9 @@ public class PrivilegeSpec extends StmtNode {
} catch (TableLoadingException e) {
throw new AnalysisException(e.getMessage(), e);
} catch (AnalysisException e) {
- throw new AnalysisException(String.format("Error setting privileges for " +
+ throw new AnalysisException(String.format("Error setting/showing privileges for " +
"table '%s'. Verify that the table exists and that you have permissions " +
- "to issue a GRANT/REVOKE statement.", tableName_.toString()));
+ "to issue a GRANT/REVOKE/SHOW GRANT statement.", tableName_.toString()));
}
Preconditions.checkNotNull(table);
return table;
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 1532965..409a8d9 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
@@ -115,6 +115,20 @@ public class AnalyzeAuthStmtsTest extends FrontendTestBase {
"Authorization is not enabled.");
AnalysisError("SHOW GRANT ROLE myRole ON SERVER", authDisabledCtx,
"Authorization is not enabled.");
+
+ // Database, table, and column do not exist.
+ AnalysisError(String.format("SHOW GRANT %s on DATABASE foo", type),
+ "Error setting/showing privileges for database 'foo'. " +
+ "Verify that the database exists and that you have permissions to issue " +
+ "a GRANT/REVOKE/SHOW GRANT statement.");
+ AnalysisError(String.format("SHOW GRANT %s on TABLE foo.bar", type),
+ "Error setting/showing privileges for table 'foo.bar'. Verify that the " +
+ "table exists and that you have permissions to issue a " +
+ "GRANT/REVOKE/SHOW GRANT statement.");
+ AnalysisError(String.format("SHOW GRANT %s on COLUMN foo.bar.baz", type),
+ "Error setting/showing privileges for table 'foo.bar'. Verify that the " +
+ "table exists and that you have permissions to issue a " +
+ "GRANT/REVOKE/SHOW GRANT statement.");
}
// Determining if a user exists on the system is done in the AuthorizationPolicy and
@@ -173,13 +187,13 @@ public class AnalyzeAuthStmtsTest extends FrontendTestBase {
AnalysisError(String.format("%s ALL ON URI 'xxxx:////abc//123' %s %s",
formatArgs), "No FileSystem for scheme: xxxx");
AnalysisError(String.format("%s ALL ON DATABASE does_not_exist %s %s",
- formatArgs), "Error setting privileges for database 'does_not_exist'. " +
- "Verify that the database exists and that you have permissions to issue " +
- "a GRANT/REVOKE statement.");
+ formatArgs), "Error setting/showing privileges for " +
+ "database 'does_not_exist'. Verify that the database exists and that you " +
+ "have permissions to issue a GRANT/REVOKE/SHOW GRANT statement.");
AnalysisError(String.format("%s ALL ON TABLE does_not_exist %s %s",
- formatArgs), "Error setting privileges for table 'does_not_exist'. " +
+ formatArgs), "Error setting/showing privileges for table 'does_not_exist'. " +
"Verify that the table exists and that you have permissions to issue " +
- "a GRANT/REVOKE statement.");
+ "a GRANT/REVOKE/SHOW GRANT statement.");
AnalysisError(String.format("%s ALL ON SERVER does_not_exist %s %s",
formatArgs), "Specified server name 'does_not_exist' does not match the " +
"configured server name 'server1'");
@@ -237,14 +251,15 @@ public class AnalyzeAuthStmtsTest extends FrontendTestBase {
"are allowed in a column privilege spec.");
// Columns/table that don't exist
AnalysisError(String.format("%s SELECT (invalid_col) ON TABLE " +
- "functional.alltypes %s %s", formatArgs), "Error setting column-level " +
- "privileges for table 'functional.alltypes'. Verify that both table and " +
- "columns exist and that you have permissions to issue a GRANT/REVOKE " +
- "statement.");
+ "functional.alltypes %s %s", formatArgs), "Error setting/showing " +
+ "column-level privileges for table 'functional.alltypes'. Verify that " +
+ "both table and columns exist and that you have permissions to issue a " +
+ "GRANT/REVOKE/SHOW GRANT statement.");
AnalysisError(String.format("%s SELECT (id, int_col) ON TABLE " +
- "functional.does_not_exist %s %s", formatArgs), "Error setting " +
+ "functional.does_not_exist %s %s", formatArgs), "Error setting/showing " +
"privileges for table 'functional.does_not_exist'. Verify that the table " +
- "exists and that you have permissions to issue a GRANT/REVOKE statement.");
+ "exists and that you have permissions to issue a " +
+ "GRANT/REVOKE/SHOW GRANT statement.");
// REFRESH privilege
AnalyzesOk(String.format(
[impala] 06/07: IMPALA-8629: (part 2) Adjust new KuduStorageHandler
package
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit ff4e0a3a459b233495e71f9bcb584ac200caf23a
Author: Grant Henke <gh...@cloudera.com>
AuthorDate: Thu Jun 6 12:30:39 2019 -0500
IMPALA-8629: (part 2) Adjust new KuduStorageHandler package
This patch changes the new KuduStorageHandler
package from “org.apache.kudu.hive” to
“org.apache.hadoop.hive.kudu”.
This is done to ensure the stand-in storage handler
can be a real storage handler when a Hive integration
is added in the future. The “org.apache.hadoop.hive”
package is the standard package all Hive storage
handlers lives under.
Additionally this patch updates the stand-in InputFormat,
OutputFormat, and SerDe entries for Kudu. This allows a
future Hive integration to read HMS tables/entries created
by Impala.
This patch also includes PlannerTest fixes to handle the
ScanToken stringification changes in the newer version
of Kudu.
Change-Id: I4d0c505643247498383472704a37d27c9e1ce473
Reviewed-on: http://gerrit.cloudera.org:8080/13541
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
bin/impala-config.sh | 4 ++--
.../org/apache/impala/catalog/HdfsFileFormat.java | 7 +++---
.../java/org/apache/impala/catalog/KuduTable.java | 9 +------
.../apache/impala/analysis/AnalyzeKuduDDLTest.java | 20 ++++++++--------
.../java/org/apache/impala/analysis/ToSqlTest.java | 5 ++--
.../org/apache/impala/planner/PlannerTestBase.java | 7 ++++--
.../queries/PlannerTest/kudu.test | 28 +++++++++++-----------
tests/custom_cluster/test_kudu.py | 10 ++++----
tests/query_test/test_kudu.py | 5 ++--
9 files changed, 45 insertions(+), 50 deletions(-)
diff --git a/bin/impala-config.sh b/bin/impala-config.sh
index 2386bbd..86d9c0d 100755
--- a/bin/impala-config.sh
+++ b/bin/impala-config.sh
@@ -162,7 +162,7 @@ fi
: ${IMPALA_TOOLCHAIN_HOST:=native-toolchain.s3.amazonaws.com}
export IMPALA_TOOLCHAIN_HOST
export CDH_MAJOR_VERSION=6
-export CDH_BUILD_NUMBER=1137441
+export CDH_BUILD_NUMBER=1173663
export CDP_BUILD_NUMBER=1153860
export CDH_HADOOP_VERSION=3.0.0-cdh6.x-SNAPSHOT
export CDP_HADOOP_VERSION=3.1.1.7.0.0.0-107
@@ -666,7 +666,7 @@ if $USE_CDH_KUDU; then
export IMPALA_KUDU_VERSION=${IMPALA_KUDU_VERSION-"1.10.0-cdh6.x-SNAPSHOT"}
export IMPALA_KUDU_HOME=${CDH_COMPONENTS_HOME}/kudu-$IMPALA_KUDU_VERSION
else
- export IMPALA_KUDU_VERSION=${IMPALA_KUDU_VERSION-"84086fe"}
+ export IMPALA_KUDU_VERSION=${IMPALA_KUDU_VERSION-"988296d"}
export IMPALA_KUDU_HOME=${IMPALA_TOOLCHAIN}/kudu-$IMPALA_KUDU_VERSION
fi
export IMPALA_KUDU_JAVA_HOME=${CDH_COMPONENTS_HOME}/kudu-$IMPALA_KUDU_VERSION
diff --git a/fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java b/fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java
index 25684cb..2cdbc30 100644
--- a/fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java
+++ b/fe/src/main/java/org/apache/impala/catalog/HdfsFileFormat.java
@@ -67,9 +67,10 @@ public enum HdfsFileFormat {
"org.apache.hadoop.hive.ql.io.orc.OrcOutputFormat",
"org.apache.hadoop.hive.ql.io.orc.OrcSerde",
true, true, false),
- KUDU("org.apache.kudu.mapreduce.KuduTableInputFormat",
- "org.apache.kudu.mapreduce.KuduTableOutputFormat",
- "", false, false, false);
+ KUDU("org.apache.hadoop.hive.kudu.KuduInputFormat",
+ "org.apache.hadoop.hive.kudu.KuduOutputFormat",
+ "org.apache.hadoop.hive.kudu.KuduSerDe",
+ false, false, false);
private final String inputFormat_;
private final String outputFormat_;
diff --git a/fe/src/main/java/org/apache/impala/catalog/KuduTable.java b/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
index 1626236..329c5aa 100644
--- a/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
@@ -85,12 +85,6 @@ public class KuduTable extends Table implements FeKuduTable {
// Kudu specific value for the storage handler table property keyed by
// KEY_STORAGE_HANDLER.
public static final String KUDU_STORAGE_HANDLER =
- "org.apache.kudu.hive.KuduStorageHandler";
-
- // TODO(IMPALA-8629): Remove this after Kudu adjusts its StorageHandler logic.
- // Once that is done KUDU_STORAGE_HANDLER will be
- // "org.apache.hadoop.hive.kudu.KuduStorageHandler".
- public static final String TEMP_KUDU_STORAGE_HANDLER =
"org.apache.hadoop.hive.kudu.KuduStorageHandler";
// Key to specify the number of tablet replicas.
@@ -146,8 +140,7 @@ public class KuduTable extends Table implements FeKuduTable {
public static boolean isKuduStorageHandler(String handler) {
return handler != null && (handler.equals(KUDU_LEGACY_STORAGE_HANDLER) ||
- handler.equals(KUDU_STORAGE_HANDLER) ||
- handler.equals(TEMP_KUDU_STORAGE_HANDLER));
+ handler.equals(KUDU_STORAGE_HANDLER));
}
public static boolean isKuduTable(org.apache.hadoop.hive.metastore.api.Table msTbl) {
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
index 20e8f4e..6dfa163 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java
@@ -198,14 +198,13 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase {
"partition 30 < values) stored as kudu tblproperties" +
"('kudu.master_addresses' = '%s')", kuduMasters));
// Not using the STORED AS KUDU syntax to specify a Kudu table
- // TODO(IMPALA-8629): Uncomment tests after Kudu adjusts its StorageHandler logic.
- // AnalysisError("create table tab (x int) tblproperties (" +
- // "'storage_handler'='org.apache.kudu.hive.KuduStorageHandler')",
- // CreateTableStmt.KUDU_STORAGE_HANDLER_ERROR_MESSAGE);
+ AnalysisError("create table tab (x int) tblproperties (" +
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler')",
+ CreateTableStmt.KUDU_STORAGE_HANDLER_ERROR_MESSAGE);
// Creating unpartitioned table results in a warning.
- // AnalyzesOk("create table tab (x int primary key) stored as kudu " +
- // "tblproperties ('storage_handler'='org.apache.kudu.hive.KuduStorageHandler')",
- // "Unpartitioned Kudu tables are inefficient for large data sizes.");
+ AnalyzesOk("create table tab (x int primary key) stored as kudu tblproperties (" +
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler')",
+ "Unpartitioned Kudu tables are inefficient for large data sizes.");
// Invalid value for number of replicas
AnalysisError("create table t (x int primary key) stored as kudu tblproperties (" +
"'kudu.num_tablet_replicas'='1.1')",
@@ -445,14 +444,15 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase {
// Use all allowed optional table props.
AnalyzesOk(String.format("create external table t stored as kudu tblproperties (" +
"'kudu.table_name'='tab', 'kudu.master_addresses' = '%s', " +
- "'storage_handler'='org.apache.kudu.hive.KuduStorageHandler')", kuduMasters));
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler')",
+ kuduMasters));
// Kudu table should be specified using the STORED AS KUDU syntax.
AnalysisError("create external table t tblproperties (" +
"'storage_handler'='com.cloudera.kudu.hive.KuduStorageHandler'," +
"'kudu.table_name'='t')",
CreateTableStmt.KUDU_STORAGE_HANDLER_ERROR_MESSAGE);
AnalysisError("create external table t tblproperties (" +
- "'storage_handler'='org.apache.kudu.hive.KuduStorageHandler'," +
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler'," +
"'kudu.table_name'='t')",
CreateTableStmt.KUDU_STORAGE_HANDLER_ERROR_MESSAGE);
// Columns should not be specified in an external Kudu table
@@ -468,7 +468,7 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase {
"'storage_handler'='com.cloudera.kudu.hive.KuduStorageHandler'," +
"'kudu.table_name'='t')", CreateTableStmt.KUDU_STORAGE_HANDLER_ERROR_MESSAGE);
AnalysisError("create external table t (x int) stored as parquet tblproperties (" +
- "'storage_handler'='org.apache.kudu.hive.KuduStorageHandler'," +
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler'," +
"'kudu.table_name'='t')", CreateTableStmt.KUDU_STORAGE_HANDLER_ERROR_MESSAGE);
AnalysisError("create external table t stored as kudu tblproperties (" +
"'storage_handler'='foo', 'kudu.table_name'='t')",
diff --git a/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java b/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
index 78eb8e6..5e7c92b 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
@@ -338,7 +338,8 @@ public class ToSqlTest extends FrontendTestBase {
String.format("CREATE TABLE default.p ( a BIGINT PRIMARY KEY, b TIMESTAMP " +
"DEFAULT '1987-05-19' ) PARTITION BY HASH (a) PARTITIONS 3 " +
"STORED AS KUDU TBLPROPERTIES ('kudu.master_addresses'='%s', " +
- "'storage_handler'='org.apache.kudu.hive.KuduStorageHandler')", kuduMasters),
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler')",
+ kuduMasters),
true);
}
@@ -372,7 +373,7 @@ public class ToSqlTest extends FrontendTestBase {
String.format("CREATE TABLE default.p PRIMARY KEY (a, b) " +
"PARTITION BY HASH (a) PARTITIONS 3, RANGE (b) (PARTITION VALUE = 1) " +
"STORED AS KUDU TBLPROPERTIES ('kudu.master_addresses'='%s', " +
- "'storage_handler'='org.apache.kudu.hive.KuduStorageHandler') AS SELECT " +
+ "'storage_handler'='org.apache.hadoop.hive.kudu.KuduStorageHandler') AS SELECT " +
"int_col a, bigint_col b FROM functional.alltypes", kuduMasters), true);
}
diff --git a/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java b/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
index 3adb01f..430c352 100644
--- a/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
+++ b/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
@@ -315,8 +315,11 @@ public class PlannerTestBase extends FrontendTestBase {
Preconditions.checkNotNull(kuduClient_,
"Test should not be invoked on platforms that do not support Kudu.");
try {
- result.append(KuduScanToken.stringifySerializedToken(
- locations.scan_range.kudu_scan_token.array(), kuduClient_));
+ String token = KuduScanToken.stringifySerializedToken(
+ locations.scan_range.kudu_scan_token.array(), kuduClient_);
+ // Don't match against the table id as its nondeterministic.
+ token = token.replaceAll(" table-id=.*?,", "");
+ result.append(token);
} catch (IOException e) {
throw new IllegalStateException("Unable to parse Kudu scan token", e);
}
diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test b/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
index d8b50dc..3258fae 100644
--- a/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
+++ b/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
@@ -6,9 +6,9 @@ PLAN-ROOT SINK
row-size=28B cardinality=0
---- SCANRANGELOCATIONS
NODE 0:
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
---- DISTRIBUTEDPLAN
PLAN-ROOT SINK
|
@@ -26,9 +26,9 @@ PLAN-ROOT SINK
row-size=28B cardinality=0
---- SCANRANGELOCATIONS
NODE 0:
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
---- DISTRIBUTEDPLAN
PLAN-ROOT SINK
|
@@ -147,7 +147,7 @@ PLAN-ROOT SINK
row-size=28B cardinality=0
---- SCANRANGELOCATIONS
NODE 0:
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
---- DISTRIBUTEDPLAN
PLAN-ROOT SINK
|
@@ -181,7 +181,7 @@ PLAN-ROOT SINK
row-size=28B cardinality=0
---- SCANRANGELOCATIONS
NODE 0:
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
---- DISTRIBUTEDPLAN
PLAN-ROOT SINK
|
@@ -204,9 +204,9 @@ PLAN-ROOT SINK
row-size=28B cardinality=0
---- SCANRANGELOCATIONS
NODE 0:
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
---- DISTRIBUTEDPLAN
PLAN-ROOT SINK
|
@@ -230,9 +230,9 @@ PLAN-ROOT SINK
row-size=28B cardinality=0
---- SCANRANGELOCATIONS
NODE 0:
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
- ScanToken{table=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1004), (int64 id=1008))}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [(int64 id=1008), <end>)}
+ ScanToken{table-name=impala::functional_kudu.testtbl, range-partition: [<start>, (int64 id=1004))}
---- DISTRIBUTEDPLAN
PLAN-ROOT SINK
|
diff --git a/tests/custom_cluster/test_kudu.py b/tests/custom_cluster/test_kudu.py
index eb87a1e..38ce51b 100644
--- a/tests/custom_cluster/test_kudu.py
+++ b/tests/custom_cluster/test_kudu.py
@@ -168,9 +168,8 @@ class TestKuduHMSIntegration(CustomClusterTestSuite, KuduTestSuite):
assert ["", "EXTERNAL", "TRUE"] in table_desc
assert ["", "kudu.master_addresses", KUDU_MASTER_HOSTS] in table_desc
assert ["", "kudu.table_name", kudu_table.name] in table_desc
- # TODO(IMPALA-8629): Uncomment tests after Kudu adjusts its StorageHandler logic.
- # assert ["", "storage_handler", "org.apache.kudu.hive.KuduStorageHandler"] \
- # in table_desc
+ assert ["", "storage_handler", "org.apache.hadoop.hive.kudu.KuduStorageHandler"] \
+ in table_desc
@pytest.mark.execute_serially
def test_implicit_managed_table_props(self, cursor, kudu_client, unique_database):
@@ -191,9 +190,8 @@ class TestKuduHMSIntegration(CustomClusterTestSuite, KuduTestSuite):
assert any("kudu.master_addresses" in s for s in table_desc)
assert ["Table Type:", "MANAGED_TABLE", None] in table_desc
assert ["", "kudu.table_name", "%s.foo" % unique_database] in table_desc
- # TODO(IMPALA-8629): Uncomment tests after Kudu adjusts its StorageHandler logic.
- # assert ["", "storage_handler", "org.apache.kudu.hive.KuduStorageHandler"] \
- # in table_desc
+ assert ["", "storage_handler", "org.apache.hadoop.hive.kudu.KuduStorageHandler"] \
+ in table_desc
@pytest.mark.execute_serially
def test_drop_non_empty_db(self, unique_cursor, kudu_client):
diff --git a/tests/query_test/test_kudu.py b/tests/query_test/test_kudu.py
index 875b5d0..df347c6 100644
--- a/tests/query_test/test_kudu.py
+++ b/tests/query_test/test_kudu.py
@@ -560,9 +560,8 @@ class TestCreateExternalTable(KuduTestSuite):
assert ["", "EXTERNAL", "TRUE"] in table_desc
assert ["", "kudu.master_addresses", KUDU_MASTER_HOSTS] in table_desc
assert ["", "kudu.table_name", kudu_table.name] in table_desc
- # TODO(IMPALA-8629): Uncomment tests after Kudu adjusts its StorageHandler logic.
- # assert ["", "storage_handler", "org.apache.kudu.hive.KuduStorageHandler"] \
- # in table_desc
+ assert ["", "storage_handler", "org.apache.hadoop.hive.kudu.KuduStorageHandler"] \
+ in table_desc
@SkipIfKudu.hms_integration_enabled
def test_col_types(self, cursor, kudu_client):
[impala] 04/07: IMPALA-8588: Fix revoke grant option with Ranger
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 5b0c098b080515cc928984a46ddc973021964f90
Author: Austin Nobis <an...@cloudera.com>
AuthorDate: Tue May 28 10:44:59 2019 -0700
IMPALA-8588: Fix revoke grant option with Ranger
Previously, the REVOKE GRANT OPTION type statements would not only
revoke the grant option, but also the privilege as well. The behavior
has been updated to only revoke the grant option. In Ranger UI, this is
seen as the delegate admin option. Examples:
REVOKE SELECT ON DATABASE <database> FROM USER <user>
This will revoke the SELECT privilege on the database resource, but
if there are other privileges on that resource the grant option will
remain for those privileges.
REVOKE GRANT OPTION FOR SELECT ON DATABASE <database> FROM USER <user>
This will revoke the grant option for all privileges on this database
resource. It will not revoke the SELECT privilege on the resource.
Testing:
- Ran all FE tests
- Ran all E2E tests
- Updated test_ranger to test behavior for when REVOKE GRANT OPTION
statements are submitted.
Change-Id: Iddfccb442c3be3c266dbc2d8ae85c5674c534d7c
Reviewed-on: http://gerrit.cloudera.org:8080/13450
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../ranger/RangerCatalogdAuthorizationManager.java | 32 ++++++++++++----------
.../authorization/AuthorizationTestBase.java | 12 ++++----
tests/authorization/test_ranger.py | 29 ++++++++++++++++----
3 files changed, 47 insertions(+), 26 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
index 0b296d0..0b1527f 100644
--- a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
+++ b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
@@ -122,8 +122,8 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
public void grantPrivilegeToUser(User requestingUser, TGrantRevokePrivParams params,
TDdlExecResponse response) throws ImpalaException {
List<GrantRevokeRequest> requests = createGrantRevokeRequests(
- requestingUser.getName(), params.getPrincipal_name(), Collections.emptyList(),
- plugin_.get().getClusterName(), params.getPrivileges());
+ requestingUser.getName(), true, params.getPrincipal_name(),
+ Collections.emptyList(), plugin_.get().getClusterName(), params.getPrivileges());
grantPrivilege(requests);
refreshAuthorization(response);
@@ -133,8 +133,8 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
public void revokePrivilegeFromUser(User requestingUser, TGrantRevokePrivParams params,
TDdlExecResponse response) throws ImpalaException {
List<GrantRevokeRequest> requests = createGrantRevokeRequests(
- requestingUser.getName(), params.getPrincipal_name(), Collections.emptyList(),
- plugin_.get().getClusterName(), params.getPrivileges());
+ requestingUser.getName(), false, params.getPrincipal_name(),
+ Collections.emptyList(), plugin_.get().getClusterName(), params.getPrivileges());
revokePrivilege(requests);
refreshAuthorization(response);
@@ -144,7 +144,7 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
public void grantPrivilegeToGroup(User requestingUser, TGrantRevokePrivParams params,
TDdlExecResponse response) throws ImpalaException {
List<GrantRevokeRequest> requests = createGrantRevokeRequests(
- requestingUser.getName(), null,
+ requestingUser.getName(), true, null,
Collections.singletonList(params.getPrincipal_name()),
plugin_.get().getClusterName(), params.getPrivileges());
@@ -156,7 +156,7 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
public void revokePrivilegeFromGroup(User requestingUser, TGrantRevokePrivParams params,
TDdlExecResponse response) throws ImpalaException {
List<GrantRevokeRequest> requests = createGrantRevokeRequests(
- requestingUser.getName(), null,
+ requestingUser.getName(), false, null,
Collections.singletonList(params.getPrincipal_name()),
plugin_.get().getClusterName(), params.getPrivileges());
@@ -236,13 +236,14 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
}
public static List<GrantRevokeRequest> createGrantRevokeRequests(String grantor,
- String user, List<String> groups, String clusterName, List<TPrivilege> privileges) {
+ boolean isGrant, String user, List<String> groups, String clusterName,
+ List<TPrivilege> privileges) {
List<GrantRevokeRequest> requests = new ArrayList<>();
for (TPrivilege p: privileges) {
Function<Map<String, String>, GrantRevokeRequest> createRequest = (resource) ->
createGrantRevokeRequest(grantor, user, groups, clusterName, p.has_grant_opt,
- p.privilege_level, resource);
+ isGrant, p.privilege_level, resource);
// Ranger Impala service definition defines 3 resources:
// [DB -> Table -> Column]
@@ -271,22 +272,25 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager
}
private static GrantRevokeRequest createGrantRevokeRequest(String grantor, String user,
- List<String> groups, String clusterName, boolean withGrantOpt,
+ List<String> groups, String clusterName, boolean withGrantOpt, boolean isGrant,
TPrivilegeLevel level, Map<String, String> resource) {
GrantRevokeRequest request = new GrantRevokeRequest();
request.setGrantor(grantor);
if (user != null) request.getUsers().add(user);
if (!groups.isEmpty()) request.getGroups().addAll(groups);
- request.setDelegateAdmin((withGrantOpt) ? Boolean.TRUE : Boolean.FALSE);
+ request.setDelegateAdmin(isGrant == withGrantOpt);
request.setEnableAudit(Boolean.TRUE);
request.setReplaceExistingPermissions(Boolean.FALSE);
request.setClusterName(clusterName);
request.setResource(resource);
- if (level == TPrivilegeLevel.INSERT) {
- request.getAccessTypes().add(RangerAuthorizationChecker.UPDATE_ACCESS_TYPE);
- } else {
- request.getAccessTypes().add(level.name().toLowerCase());
+ // For revoke grant option, omit the privilege
+ if (!(!isGrant && withGrantOpt)) {
+ if (level == TPrivilegeLevel.INSERT) {
+ request.getAccessTypes().add(RangerAuthorizationChecker.UPDATE_ACCESS_TYPE);
+ } else {
+ request.getAccessTypes().add(level.name().toLowerCase());
+ }
}
return request;
diff --git a/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java b/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
index bd3b8cc..b559ba1 100644
--- a/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
+++ b/fe/src/test/java/org/apache/impala/authorization/AuthorizationTestBase.java
@@ -200,14 +200,14 @@ public abstract class AuthorizationTestBase extends FrontendTestBase {
}
protected abstract class WithRanger implements WithPrincipal {
- private final List<GrantRevokeRequest> requests = new ArrayList<>();
+ private final List<GrantRevokeRequest> grants = new ArrayList<>();
private final RangerCatalogdAuthorizationManager authzManager =
new RangerCatalogdAuthorizationManager(() -> rangerImpalaPlugin_, null);
@Override
public void init(TPrivilege[]... privileges) throws ImpalaException {
for (TPrivilege[] privilege : privileges) {
- requests.addAll(buildRequest(Arrays.asList(privilege))
+ grants.addAll(buildRequest(Arrays.asList(privilege))
.stream()
.peek(r -> {
r.setResource(updateUri(r.getResource()));
@@ -218,7 +218,7 @@ public abstract class AuthorizationTestBase extends FrontendTestBase {
}).collect(Collectors.toList()));
}
- authzManager.grantPrivilege(requests);
+ authzManager.grantPrivilege(grants);
rangerImpalaPlugin_.refreshPoliciesAndTags();
}
@@ -229,7 +229,7 @@ public abstract class AuthorizationTestBase extends FrontendTestBase {
@Override
public void cleanUp() throws ImpalaException {
- authzManager.revokePrivilege(requests);
+ authzManager.revokePrivilege(grants);
}
@Override
@@ -240,7 +240,7 @@ public abstract class AuthorizationTestBase extends FrontendTestBase {
@Override
protected List<GrantRevokeRequest> buildRequest(List<TPrivilege> privileges) {
return RangerCatalogdAuthorizationManager.createGrantRevokeRequests(
- RANGER_ADMIN.getName(), USER.getName(), Collections.emptyList(),
+ RANGER_ADMIN.getName(), true, USER.getName(), Collections.emptyList(),
rangerImpalaPlugin_.getClusterName(), privileges);
}
}
@@ -251,7 +251,7 @@ public abstract class AuthorizationTestBase extends FrontendTestBase {
List<String> groups = Collections.singletonList(System.getProperty("user.name"));
return RangerCatalogdAuthorizationManager.createGrantRevokeRequests(
- RANGER_ADMIN.getName(), null, groups,
+ RANGER_ADMIN.getName(), true, null, groups,
rangerImpalaPlugin_.getClusterName(), privileges);
}
}
diff --git a/tests/authorization/test_ranger.py b/tests/authorization/test_ranger.py
index bb1019a..4f3ac87 100644
--- a/tests/authorization/test_ranger.py
+++ b/tests/authorization/test_ranger.py
@@ -124,25 +124,44 @@ class TestRanger(CustomClusterTestSuite):
"grant select on database {0} to user {1} with "
"grant option".format(unique_database, user1),
user=ADMIN)
+ self.execute_query_expect_success(admin_client,
+ "grant insert on database {0} to user {1} with "
+ "grant option".format(unique_database, user1),
+ user=ADMIN)
# Verify user 1 has with_grant privilege on unique_database
result = self.execute_query("show grant user {0} on database {1}"
.format(user1, unique_database))
TestRanger._check_privileges(result, [
+ ["USER", user1, unique_database, "", "", "", "*", "insert", "true"],
["USER", user1, unique_database, "", "", "", "*", "select", "true"],
+ ["USER", user1, unique_database, "*", "*", "", "", "insert", "true"],
["USER", user1, unique_database, "*", "*", "", "", "select", "true"]])
+ # Revoke select privilege and check grant option is still present
+ self.execute_query_expect_success(admin_client,
+ "revoke select on database {0} from user {1}"
+ .format(unique_database, user1), user=ADMIN)
+ result = self.execute_query("show grant user {0} on database {1}"
+ .format(user1, unique_database))
+ TestRanger._check_privileges(result, [
+ ["USER", user1, unique_database, "", "", "", "*", "insert", "true"],
+ ["USER", user1, unique_database, "*", "*", "", "", "insert", "true"]])
+
# Revoke privilege granting from user 1
- self.execute_query_expect_success(admin_client, "revoke grant option for select "
+ self.execute_query_expect_success(admin_client, "revoke grant option for insert "
"on database {0} from user {1}"
.format(unique_database, user1), user=ADMIN)
# User 1 can no longer grant privileges on unique_database
+ # In ranger it is currently not possible to revoke grant for a single access type
result = self.execute_query("show grant user {0} on database {1}"
.format(user1, unique_database))
- TestRanger._check_privileges(result, [])
+ TestRanger._check_privileges(result, [
+ ["USER", user1, unique_database, "", "", "", "*", "insert", "false"],
+ ["USER", user1, unique_database, "*", "*", "", "", "insert", "false"]])
finally:
- admin_client.execute("revoke grant option for select on database {0} from user {1}"
+ admin_client.execute("revoke insert on database {0} from user {1}"
.format(unique_database, user1), user=ADMIN)
admin_client.execute("drop database if exists {0} cascade".format(unique_database),
user=ADMIN)
@@ -177,9 +196,6 @@ class TestRanger(CustomClusterTestSuite):
# Test USER inherits privileges for their GROUP
self._test_show_grant_user_group(admin_client, user, group, unique_db)
-
- # Test that show grant without ON a resource fails
-
finally:
admin_client.execute("drop database if exists {0} cascade".format(unique_db),
user=ADMIN)
@@ -237,6 +253,7 @@ class TestRanger(CustomClusterTestSuite):
["USER", user, "*", "", "", "", "*", "all", "false"],
["USER", user, "*", "*", "*", "", "", "all", "false"]])
finally:
+ admin_client.execute("revoke all on server from user {0}".format(user))
for privilege in privileges:
admin_client.execute("revoke {0} on server from user {1}".format(privilege, user))
[impala] 02/07: IMPALA-8436: Prohibit write/alter operations on
materialized view
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit fcaec278ccf4638692701fd03309a4f40e8fd611
Author: arorasudhanshu <su...@cloudera.com>
AuthorDate: Mon Jun 3 15:40:40 2019 -0700
IMPALA-8436: Prohibit write/alter operations on materialized view
Instead of creating an in memory instance of View, we were
creating instance of HdfsTable. Modified the code to create instance of
View for materialized view.
Testing Done:
- Added tests in AnalyzerTest.
Change-Id: Idcd619303e19b5a2551876a63d67569c76bd22f0
Reviewed-on: http://gerrit.cloudera.org:8080/13503
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
---
.../org/apache/impala/compat/MetastoreShim.java | 24 ++++++++++++++++++++++
.../main/java/org/apache/impala/catalog/Table.java | 4 +++-
.../org/apache/impala/analysis/AnalyzerTest.java | 19 +++++++++++++++++
.../functional/functional_schema_template.sql | 10 +++++++++
4 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java b/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
index c79dca9..de58116 100644
--- a/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
+++ b/fe/src/compat-hive-2/java/org/apache/impala/compat/MetastoreShim.java
@@ -159,6 +159,30 @@ public class MetastoreShim {
.put("INDEX_TABLE", TABLE_TYPE_TABLE)
.put("VIRTUAL_VIEW", TABLE_TYPE_VIEW).build();
+ public static String mapToInternalTableType(String typeStr) {
+ String defaultTableType = TABLE_TYPE_TABLE;
+
+ TableType tType;
+
+ if (typeStr == null) return defaultTableType;
+ try {
+ tType = TableType.valueOf(typeStr.toUpperCase());
+ } catch (Exception e) {
+ return defaultTableType;
+ }
+ switch (tType) {
+ case EXTERNAL_TABLE:
+ case MANAGED_TABLE:
+ case INDEX_TABLE:
+ return TABLE_TYPE_TABLE;
+ case VIRTUAL_VIEW:
+ return TABLE_TYPE_VIEW;
+ default:
+ return defaultTableType;
+ }
+
+ }
+
/**
* Wrapper method which returns ExtendedJSONMessageFactory in case Impala is
* building against Hive-2 to keep compatibility with Sentry
diff --git a/fe/src/main/java/org/apache/impala/catalog/Table.java b/fe/src/main/java/org/apache/impala/catalog/Table.java
index ab44c55..f3ddd63 100644
--- a/fe/src/main/java/org/apache/impala/catalog/Table.java
+++ b/fe/src/main/java/org/apache/impala/catalog/Table.java
@@ -39,6 +39,7 @@ import org.apache.impala.common.ImpalaRuntimeException;
import org.apache.impala.common.Metrics;
import org.apache.impala.common.Pair;
import org.apache.impala.common.RuntimeEnv;
+import org.apache.impala.service.MetadataOp;
import org.apache.impala.thrift.TAccessLevel;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
@@ -327,7 +328,8 @@ public abstract class Table extends CatalogObjectImpl implements FeTable {
CatalogInterners.internFieldsInPlace(msTbl);
Table table = null;
// Create a table of appropriate type
- if (TableType.valueOf(msTbl.getTableType()) == TableType.VIRTUAL_VIEW) {
+ if (MetadataOp.TABLE_TYPE_VIEW.equals(
+ MetastoreShim.mapToInternalTableType(msTbl.getTableType()))) {
table = new View(msTbl, db, msTbl.getTableName(), msTbl.getOwner());
} else if (HBaseTable.isHBaseTable(msTbl)) {
table = new HBaseTable(msTbl, db, msTbl.getTableName(), msTbl.getOwner());
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
index 4177bf7..d5f42f8 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
@@ -615,6 +615,25 @@ public class AnalyzerTest extends FrontendTestBase {
AnalyzesOk("show column stats functional.insert_only_transactional_table");
}
+ @Test
+ public void TestAnalyzeMaterializedView() {
+ Assume.assumeTrue(MetastoreShim.getMajorVersion() > 2);
+ AnalysisError("alter table functional.materialized_view " +
+ "set tblproperties ('foo'='bar')",
+ "ALTER TABLE not allowed on a " +
+ "view: functional.materialized_view");
+
+ AnalysisError("insert into table functional.materialized_view " +
+ "select * from functional.insert_only_transactional_table",
+ "Impala does not support INSERTing into views:" +
+ " functional.materialized_view");
+
+ AnalysisError("drop table functional.materialized_view ",
+ "DROP TABLE not allowed on a view: functional.materialized_view");
+
+ AnalyzesOk("Select * from functional.materialized_view");
+ }
+
private Function createFunction(boolean hasVarArgs, Type... args) {
return new Function(new FunctionName("test"), args, Type.INVALID, hasVarArgs);
}
diff --git a/testdata/datasets/functional/functional_schema_template.sql b/testdata/datasets/functional/functional_schema_template.sql
index 3de8363..79d551d 100644
--- a/testdata/datasets/functional/functional_schema_template.sql
+++ b/testdata/datasets/functional/functional_schema_template.sql
@@ -2152,6 +2152,16 @@ TBLPROPERTIES('transactional'='true');
---- DATASET
functional
---- BASE_TABLE_NAME
+materialized_view
+---- HIVE_MAJOR_VERSION
+3
+---- CREATE_HIVE
+CREATE MATERIALIZED VIEW IF NOT EXISTS {db_name}{db_suffix}.{table_name}
+ AS SELECT * FROM {db_name}{db_suffix}.insert_only_transactional_table;
+====
+---- DATASET
+functional
+---- BASE_TABLE_NAME
testescape_16_lf
---- CREATE
CREATE EXTERNAL TABLE IF NOT EXISTS {db_name}{db_suffix}.{table_name} (
[impala] 05/07: IMPALA-7154: Increase metastore client time out for
tests
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit e3117aa6645fec89ef587d6e57b38b981080cb03
Author: Vihang Karajgaonkar <vi...@cloudera.com>
AuthorDate: Tue Jun 11 14:24:26 2019 -0700
IMPALA-7154: Increase metastore client time out for tests
Some of the e2e tests create tables and databases on S3. HMS operations
like dropDatabaseCascade can take significant time (3-5 min) in such
tests. If the HMS Client times out, it retries for a configurable number
of times. Unfortunately, HMS client is not smart enough to understand
which operations should be retried. Eg, a drop database operation which
is timedout on the client side, should not be retried since the server
thread may have succeeded before the next retry. This would cause the
client to receive a NoSuchObjectException which would look like a flaky
test failure on the Impala side.
This patch increases the metastore client.socket.timeout for
the tests so that such flaky test failures are reduced.
Note:
1. The timeout value of 10 min was chosen based on looking at logs for
one such failure where the HMS API took about 3.5 min. Once this patch
is deployed we should continue to see if such failures reoccur and
readjust the client timeout value accordingly.
Change-Id: I253fc00618b28d8d6e626e1b6bf9c5c8290006d7
Reviewed-on: http://gerrit.cloudera.org:8080/13586
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
fe/src/test/resources/hive-site.xml.py | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/fe/src/test/resources/hive-site.xml.py b/fe/src/test/resources/hive-site.xml.py
index e46adc3..d8acccd 100644
--- a/fe/src/test/resources/hive-site.xml.py
+++ b/fe/src/test/resources/hive-site.xml.py
@@ -31,8 +31,15 @@ CONFIG.update({
# IMPALA-781: Impala doesn't support the default LazyBinaryColumnarSerde for RCFile.
'hive.default.rcfile.serde': 'org.apache.hadoop.hive.serde2.columnar.ColumnarSerDe',
- 'hive.metastore.client.connect.retry.delay': '0',
- 'hive.metastore.client.socket.timeout': '120',
+ # IMPALA-7154: Some of the HMS operations on S3 can take long time and any timeouts
+ # on the HMS Client side can result in flaky test failures
+ # Following configuration specifies the time in seconds between successive retry
+ # attempts by metastore client in case of failures. By default, metastore client
+ # retries once based on the config value of hive.metastore.failure.retries
+ 'hive.metastore.client.connect.retry.delay': '1',
+ # set the metastore client timeout to 10 min
+ 'hive.metastore.client.socket.timeout': '600',
+
'hive.metastore.uris': 'thrift://${INTERNAL_LISTEN_HOST}:9083',
'hive.metastore.warehouse.dir': '${WAREHOUSE_LOCATION_PREFIX}/test-warehouse',
[impala] 07/07: IMPALA-8143: Enhance DoRpcWithRetry().
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 94652d74521e95e8606ea2d22aabcaddde6fc471
Author: Andrew Sherman <as...@cloudera.com>
AuthorDate: Wed May 22 17:25:04 2019 -0700
IMPALA-8143: Enhance DoRpcWithRetry().
Allow callers of RpcMgr::DoRpcWithRetry to specify a time to sleep if
the remote service is busy. DoRpcWithRetry now only sleeps if the remote
service is busy.
TESTING:
Ran all end-to-end tests.
Add two new tests to rpc-mgr-test.cc which test RpcMgr::DoRpcWithRetry.
One test uses a fake Proxy class, the other uses a new DebugAction to
cause Krpc calls to be rejected as if the service was too busy.
Change-Id: Ia9693151c35e02235665b3c285a48c585973d390
Reviewed-on: http://gerrit.cloudera.org:8080/12672
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/rpc/impala-service-pool.cc | 13 ++++-
be/src/rpc/impala-service-pool.h | 3 +
be/src/rpc/rpc-mgr-test.cc | 87 ++++++++++++++++++++++++++++-
be/src/rpc/rpc-mgr-test.h | 27 +++++++--
be/src/rpc/rpc-mgr.h | 15 +++++
be/src/rpc/rpc-mgr.inline.h | 36 ++++++++++++
be/src/runtime/coordinator-backend-state.cc | 14 +++--
be/src/runtime/query-state.h | 2 +-
be/src/service/client-request-state.cc | 13 +++--
be/src/service/control-service.h | 23 --------
10 files changed, 189 insertions(+), 44 deletions(-)
diff --git a/be/src/rpc/impala-service-pool.cc b/be/src/rpc/impala-service-pool.cc
index 44a0c64..9db137a 100644
--- a/be/src/rpc/impala-service-pool.cc
+++ b/be/src/rpc/impala-service-pool.cc
@@ -50,9 +50,11 @@ METRIC_DEFINE_histogram(server, impala_incoming_queue_time,
using namespace rapidjson;
+DECLARE_string(debug_actions);
+
namespace impala {
-// Metric key format for rpc call duration metrics.
-const string RPC_QUEUE_OVERFLOW_METRIC_KEY = "rpc.$0.rpcs_queue_overflow";
+const char * ImpalaServicePool::RPC_QUEUE_OVERFLOW_METRIC_KEY =
+ "rpc.$0.rpcs_queue_overflow";
ImpalaServicePool::ImpalaServicePool(const scoped_refptr<kudu::MetricEntity>& entity,
int service_queue_length, kudu::rpc::GeneratedServiceIf* service,
@@ -186,6 +188,13 @@ kudu::Status ImpalaServicePool::QueueInboundCall(
service_mem_tracker_->Consume(transfer_size);
}
+ Status debug_status = DebugAction(FLAGS_debug_actions, "SERVICE_POOL_SERVER_BUSY");
+ if (UNLIKELY(!debug_status.ok())) {
+ // Simulate the service being too busy.
+ RejectTooBusy(c);
+ return kudu::Status::OK();
+ }
+
boost::optional<kudu::rpc::InboundCall*> evicted;
auto queue_status = service_queue_.Put(c, &evicted);
if (UNLIKELY(queue_status == kudu::rpc::QueueStatus::QUEUE_FULL)) {
diff --git a/be/src/rpc/impala-service-pool.h b/be/src/rpc/impala-service-pool.h
index b4313bf..dd56436 100644
--- a/be/src/rpc/impala-service-pool.h
+++ b/be/src/rpc/impala-service-pool.h
@@ -68,6 +68,9 @@ class ImpalaServicePool : public kudu::rpc::RpcService {
/// Expose the service pool metrics by storing them as JSON in 'value'.
void ToJson(rapidjson::Value* value, rapidjson::Document* document);
+ /// Metric key format for rpc call duration metrics.
+ static const char* RPC_QUEUE_OVERFLOW_METRIC_KEY;
+
private:
void RunThread();
void RejectTooBusy(kudu::rpc::InboundCall* c);
diff --git a/be/src/rpc/rpc-mgr-test.cc b/be/src/rpc/rpc-mgr-test.cc
index 07dbfe7..c0d4fbd 100644
--- a/be/src/rpc/rpc-mgr-test.cc
+++ b/be/src/rpc/rpc-mgr-test.cc
@@ -31,12 +31,14 @@ DECLARE_int32(num_reactor_threads);
DECLARE_int32(num_acceptor_threads);
DECLARE_int32(rpc_negotiation_timeout_ms);
DECLARE_string(hostname);
+DECLARE_string(debug_actions);
// For tests that do not require kerberized testing, we use RpcTest.
namespace impala {
+// Test multiple services managed by an Rpc Manager using TLS.
TEST_F(RpcMgrTest, MultipleServicesTls) {
- // TODO: We're starting a seperate RpcMgr here instead of configuring
+ // TODO: We're starting a separate RpcMgr here instead of configuring
// RpcTestBase::rpc_mgr_ to use TLS. To use RpcTestBase::rpc_mgr_, we need to introduce
// new gtest params to turn on TLS which needs to be a coordinated change across
// rpc-mgr-test and thrift-server-test.
@@ -55,6 +57,7 @@ TEST_F(RpcMgrTest, MultipleServicesTls) {
tls_rpc_mgr.Shutdown();
}
+// Test multiple services managed by an Rpc Manager.
TEST_F(RpcMgrTest, MultipleServices) {
ASSERT_OK(RunMultipleServicesTest(&rpc_mgr_, krpc_address_));
}
@@ -163,6 +166,7 @@ TEST_F(RpcMgrTest, ValidMultiCiphersTls) {
tls_rpc_mgr.Shutdown();
}
+// Test behavior with a slow service.
TEST_F(RpcMgrTest, SlowCallback) {
// Use a callback which is slow to respond.
auto slow_cb = [](RpcContext* ctx) {
@@ -199,6 +203,7 @@ TEST_F(RpcMgrTest, SlowCallback) {
}
}
+// Test async calls.
TEST_F(RpcMgrTest, AsyncCall) {
GeneratedServiceIf* scan_mem_impl =
TakeOverService(make_unique<ScanMemServiceImpl>(&rpc_mgr_));
@@ -251,6 +256,86 @@ TEST_F(RpcMgrTest, NegotiationTimeout) {
secondary_rpc_mgr.Shutdown();
}
+// Test RpcMgr::DoRpcWithRetry using a fake proxy.
+TEST_F(RpcMgrTest, DoRpcWithRetry) {
+ TQueryCtx query_ctx;
+ const int num_retries = 10;
+ const int64_t timeout_ms = 10 * MILLIS_PER_SEC;
+
+ // Test how DoRpcWithRetry retries by using a proxy that always fails.
+ unique_ptr<FailingPingServiceProxy> failing_proxy =
+ make_unique<FailingPingServiceProxy>();
+ // A call that fails is not retried as the server is not busy.
+ PingRequestPB request1;
+ PingResponsePB response1;
+ Status rpc_status_fail =
+ RpcMgr::DoRpcWithRetry(failing_proxy, &FailingPingServiceProxy::Ping, request1,
+ &response1, query_ctx, "ping failed", num_retries, timeout_ms);
+ ASSERT_FALSE(rpc_status_fail.ok());
+ // Check that proxy was only called once.
+ ASSERT_EQ(1, failing_proxy->GetNumberOfCalls());
+
+ // Test injection of DebugAction into DoRpcWithRetry.
+ query_ctx.client_request.query_options.__set_debug_action("DoRpcWithRetry:FAIL");
+ PingRequestPB request2;
+ PingResponsePB response2;
+ Status inject_status = RpcMgr::DoRpcWithRetry(failing_proxy,
+ &FailingPingServiceProxy::Ping, request2, &response2, query_ctx, "ping failed",
+ num_retries, timeout_ms, 0, "DoRpcWithRetry");
+ ASSERT_FALSE(inject_status.ok());
+ EXPECT_ERROR(inject_status, TErrorCode::INTERNAL_ERROR);
+ ASSERT_EQ("Debug Action: DoRpcWithRetry:FAIL", inject_status.msg().msg());
+}
+
+// Test RpcMgr::DoRpcWithRetry by injecting service-too-busy failures.
+TEST_F(RpcMgrTest, BusyService) {
+ TQueryCtx query_ctx;
+ auto cb = [](RpcContext* ctx) { ctx->RespondSuccess(); };
+ GeneratedServiceIf* ping_impl =
+ TakeOverService(make_unique<PingServiceImpl>(&rpc_mgr_, cb));
+ const int num_service_threads = 4;
+ const int queue_size = 25;
+ ASSERT_OK(rpc_mgr_.RegisterService(num_service_threads, queue_size, ping_impl,
+ static_cast<PingServiceImpl*>(ping_impl)->mem_tracker()));
+ FLAGS_num_acceptor_threads = 2;
+ FLAGS_num_reactor_threads = 10;
+ ASSERT_OK(rpc_mgr_.StartServices(krpc_address_));
+
+ // Find the counter which tracks the number of times the service queue is too full.
+ const string& overflow_count = Substitute(
+ ImpalaServicePool::RPC_QUEUE_OVERFLOW_METRIC_KEY, ping_impl->service_name());
+ IntCounter* overflow_metric =
+ ExecEnv::GetInstance()->rpc_metrics()->FindMetricForTesting<IntCounter>(
+ overflow_count);
+ ASSERT_TRUE(overflow_metric != nullptr);
+
+ unique_ptr<PingServiceProxy> proxy;
+ ASSERT_OK(static_cast<PingServiceImpl*>(ping_impl)->GetProxy(
+ krpc_address_, FLAGS_hostname, &proxy));
+
+ // There have been no overflows yet.
+ EXPECT_EQ(overflow_metric->GetValue(), 0L);
+
+ // Use DebugAction to make the Impala Service Pool reject 50% of Krpc calls as if the
+ // service is too busy.
+ auto s = ScopedFlagSetter<string>::Make(
+ &FLAGS_debug_actions, "SERVICE_POOL_SERVER_BUSY:FAIL@0.5");
+ PingRequestPB request;
+ PingResponsePB response;
+ const int64_t timeout_ms = 10 * MILLIS_PER_SEC;
+ int num_retries = 40; // How many times DoRpcWithRetry can retry.
+ int num_rpc_retry_calls = 40; // How many times to call DoRpcWithRetry
+ for (int i = 0; i < num_rpc_retry_calls; ++i) {
+ Status status = RpcMgr::DoRpcWithRetry(proxy, &PingServiceProxy::Ping, request,
+ &response, query_ctx, "ping failed", num_retries, timeout_ms);
+ // DoRpcWithRetry will fail with probability (1/2)^num_rpc_retry_calls.
+ ASSERT_TRUE(status.ok());
+ }
+ // There will be no overflows (i.e. service too busy) with probability
+ // (1/2)^num_retries.
+ ASSERT_GT(overflow_metric->GetValue(), 0);
+}
+
} // namespace impala
int main(int argc, char** argv) {
diff --git a/be/src/rpc/rpc-mgr-test.h b/be/src/rpc/rpc-mgr-test.h
index 8b749b5..ad428a4 100644
--- a/be/src/rpc/rpc-mgr-test.h
+++ b/be/src/rpc/rpc-mgr-test.h
@@ -18,8 +18,6 @@
#ifndef IMPALA_RPC_RPC_MGR_TEST_H
#define IMPALA_RPC_RPC_MGR_TEST_H
-#include "rpc/rpc-mgr.inline.h"
-
#include "common/init.h"
#include "exec/kudu-util.h"
#include "kudu/rpc/remote_user.h"
@@ -268,6 +266,25 @@ class ScanMemServiceImpl : public ScanMemServiceIf {
};
+/// A class that behaves like a ::kudu::rpc::Proxy and keeps a count of the number of
+/// times it is called. It always fails by returning an IOError.
+class FailingPingServiceProxy {
+ public:
+ kudu::Status Ping(const class PingRequestPB& req, class PingResponsePB* resp,
+ ::kudu::rpc::RpcController* controller) {
+ ++number_of_calls_;
+ return kudu::Status::IOError(
+ Substitute("ping failing, number of calls=$0.", number_of_calls_));
+ }
+
+ /// Return the number of times Ping has been called.
+ int GetNumberOfCalls() const { return number_of_calls_; }
+
+ private:
+ /// Number of times Ping has been called.
+ int number_of_calls_ = 0;
+};
+
Status RpcMgrTest::RunMultipleServicesTest(
RpcMgr* rpc_mgr, const TNetworkAddress& krpc_address) {
// Test that a service can be started, and will respond to requests.
@@ -308,9 +325,9 @@ Status RpcMgrTest::RunMultipleServicesTest(
KUDU_RETURN_IF_ERROR(ping_proxy->Ping(request, &response, &controller),
"unable to execute Ping() RPC.");
if (response.int_response() != 42) {
- return Status(Substitute(
- "Ping() failed. Incorrect response. Expected: 42; Got: $0",
- response.int_response()));
+ return Status(
+ Substitute("Ping() failed. Incorrect response. Expected: 42; Got: $0",
+ response.int_response()));
}
} else {
ScanMemRequestPB request;
diff --git a/be/src/rpc/rpc-mgr.h b/be/src/rpc/rpc-mgr.h
index 7829897..eb45203 100644
--- a/be/src/rpc/rpc-mgr.h
+++ b/be/src/rpc/rpc-mgr.h
@@ -175,6 +175,21 @@ class RpcMgr {
/// in flight, and metrics and histograms for each service and their methods.
void ToJson(rapidjson::Document* document);
+ /// Retry the Rpc 'rpc_call' on the 'proxy' object up to 'times_to_try' times.
+ /// The 'rpc_call' must be idempotent as it may be called multiple times.
+ /// Each Rpc has a timeout of 'timeout_ms' milliseconds.
+ /// If the service is busy then sleep 'server_busy_backoff_ms' milliseconds before
+ /// retrying.
+ /// Pass 'debug_action' to DebugAction() to potentially inject errors.
+ /// TODO: Clean up this interface. Replace the debug action with fault injection in RPC
+ /// callbacks or other places.
+ template <typename Proxy, typename ProxyMethod, typename Request, typename Response>
+ static Status DoRpcWithRetry(const std::unique_ptr<Proxy>& proxy,
+ const ProxyMethod& rpc_call, const Request& request, Response* response,
+ const TQueryCtx& query_ctx, const char* error_msg, const int times_to_try,
+ const int64_t timeout_ms, const int64_t server_busy_backoff_ms = 0,
+ const char* debug_action = nullptr);
+
~RpcMgr() {
DCHECK_EQ(service_pools_.size(), 0)
<< "Must call Shutdown() before destroying RpcMgr";
diff --git a/be/src/rpc/rpc-mgr.inline.h b/be/src/rpc/rpc-mgr.inline.h
index 9a1fc7e..76ee34d 100644
--- a/be/src/rpc/rpc-mgr.inline.h
+++ b/be/src/rpc/rpc-mgr.inline.h
@@ -49,6 +49,42 @@ Status RpcMgr::GetProxy(const TNetworkAddress& address, const std::string& hostn
return Status::OK();
}
+template <typename Proxy, typename ProxyMethod, typename Request, typename Response>
+Status RpcMgr::DoRpcWithRetry(const std::unique_ptr<Proxy>& proxy,
+ const ProxyMethod& rpc_call, const Request& request, Response* response,
+ const TQueryCtx& query_ctx, const char* error_msg, const int times_to_try,
+ const int64_t timeout_ms, const int64_t server_busy_backoff_ms,
+ const char* debug_action) {
+ DCHECK_GT(times_to_try, 0);
+ Status rpc_status;
+
+ for (int i = 0; i < times_to_try; ++i) {
+ kudu::rpc::RpcController rpc_controller;
+ rpc_controller.set_timeout(kudu::MonoDelta::FromMilliseconds(timeout_ms));
+
+ if (debug_action != nullptr) {
+ // Check for injected failures.
+ rpc_status = DebugAction(query_ctx.client_request.query_options, debug_action);
+ if (!rpc_status.ok()) continue;
+ }
+
+ const kudu::Status& k_status =
+ (proxy.get()->*rpc_call)(request, response, &rpc_controller);
+ rpc_status = FromKuduStatus(k_status, error_msg);
+
+ // If the call succeeded, or the server is not busy, then return result to caller.
+ if (rpc_status.ok() || !RpcMgr::IsServerTooBusy(rpc_controller)) {
+ return rpc_status;
+ }
+
+ // Server is busy, sleep if caller requested it and this is not the last time to try.
+ if (server_busy_backoff_ms != 0 && i != times_to_try - 1) {
+ SleepForMs(server_busy_backoff_ms);
+ }
+ }
+ return rpc_status;
+}
+
} // namespace impala
#endif
diff --git a/be/src/runtime/coordinator-backend-state.cc b/be/src/runtime/coordinator-backend-state.cc
index 4935a7a..bd97712 100644
--- a/be/src/runtime/coordinator-backend-state.cc
+++ b/be/src/runtime/coordinator-backend-state.cc
@@ -26,6 +26,7 @@
#include "kudu/rpc/rpc_controller.h"
#include "kudu/util/monotime.h"
#include "kudu/util/status.h"
+#include "rpc/rpc-mgr.inline.h"
#include "runtime/backend-client.h"
#include "runtime/client-cache.h"
#include "runtime/coordinator-filter-state.h"
@@ -465,12 +466,13 @@ bool Coordinator::BackendState::Cancel() {
TUniqueIdToUniqueIdPB(query_id(), request.mutable_query_id());
CancelQueryFInstancesResponsePB response;
- auto cancel_rpc = [&](RpcController* rpc_controller) -> kudu::Status {
- return proxy->CancelQueryFInstances(request, &response, rpc_controller);
- };
-
- Status rpc_status = ControlService::DoRpcWithRetry(cancel_rpc, query_ctx(),
- "COORD_CANCEL_QUERY_FINSTANCES_RPC", "Cancel() RPC failed", 3, 10);
+ const int num_retries = 3;
+ const int64_t timeout_ms = 10 * MILLIS_PER_SEC;
+ const int64_t backoff_time_ms = 3 * MILLIS_PER_SEC;
+ Status rpc_status =
+ RpcMgr::DoRpcWithRetry(proxy, &ControlServiceProxy::CancelQueryFInstances, request,
+ &response, query_ctx(), "Cancel() RPC failed", num_retries, timeout_ms,
+ backoff_time_ms, "COORD_CANCEL_QUERY_FINSTANCES_RPC");
if (!rpc_status.ok()) {
status_.MergeStatus(rpc_status);
diff --git a/be/src/runtime/query-state.h b/be/src/runtime/query-state.h
index 1f8cc18..408ba0a 100644
--- a/be/src/runtime/query-state.h
+++ b/be/src/runtime/query-state.h
@@ -417,7 +417,7 @@ class QueryState {
void ConstructReport(bool instances_started, ReportExecStatusRequestPB* report,
TRuntimeProfileForest* profiles_forest);
- /// Gather statues and profiles of all fragment instances belonging to this query state
+ /// Gather statuses and profiles of all fragment instances belonging to this query state
/// and send it to the coordinator via ReportExecStatus() RPC. Returns true if the
/// report rpc was successful or if it was unsuccessful and we've reached the maximum
/// number of allowed failures and cancelled.
diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index 1427fbb..6b2ab39 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -25,6 +25,7 @@
#include "common/status.h"
#include "exec/kudu-util.h"
#include "kudu/rpc/rpc_controller.h"
+#include "rpc/rpc-mgr.inline.h"
#include "runtime/backend-client.h"
#include "runtime/coordinator.h"
#include "runtime/exec-env.h"
@@ -658,12 +659,12 @@ Status ClientRequestState::ExecShutdownRequest() {
RemoteShutdownResultPB resp;
VLOG_QUERY << "Sending Shutdown RPC to " << TNetworkAddressToString(addr);
- auto shutdown_rpc = [&](RpcController* rpc_controller) -> kudu::Status {
- return proxy->RemoteShutdown(params, &resp, rpc_controller);
- };
-
- Status rpc_status = ControlService::DoRpcWithRetry(
- shutdown_rpc, query_ctx_, "CRS_SHUTDOWN_RPC", "RemoteShutdown() RPC failed", 3, 10);
+ const int num_retries = 3;
+ const int64_t timeout_ms = 10 * MILLIS_PER_SEC;
+ const int64_t backoff_time_ms = 3 * MILLIS_PER_SEC;
+ Status rpc_status = RpcMgr::DoRpcWithRetry(proxy, &ControlServiceProxy::RemoteShutdown,
+ params, &resp, query_ctx_, "RemoteShutdown() RPC failed", num_retries, timeout_ms,
+ backoff_time_ms, "CRS_SHUTDOWN_RPC");
if (!rpc_status.ok()) {
const string& msg = rpc_status.msg().msg();
diff --git a/be/src/service/control-service.h b/be/src/service/control-service.h
index 6a0267f..4a869e0 100644
--- a/be/src/service/control-service.h
+++ b/be/src/service/control-service.h
@@ -76,29 +76,6 @@ class ControlService : public ControlServiceIf {
static Status GetProxy(const TNetworkAddress& address, const std::string& hostname,
std::unique_ptr<ControlServiceProxy>* proxy);
- /// Retry the Rpc 'rpc_call' up to 'times_to_try' times.
- /// Each Rpc has a timeout of 'timeout_s' seconds.
- /// There is no sleeping between retries.
- /// Pass 'debug_action' to DebugAction() to potentially inject errors.
- template <typename F>
- static Status DoRpcWithRetry(F&& rpc_call, const TQueryCtx& query_ctx,
- const char* debug_action, const char* error_msg, int times_to_try, int timeout_s) {
- DCHECK_GT(times_to_try, 0);
- Status rpc_status;
- for (int i = 0; i < times_to_try; i++) {
- RpcController rpc_controller;
- rpc_controller.set_timeout(MonoDelta::FromSeconds(timeout_s));
- // Check for injected failures.
- rpc_status = DebugAction(query_ctx.client_request.query_options, debug_action);
- if (!rpc_status.ok()) continue;
-
- rpc_status = FromKuduStatus(rpc_call(&rpc_controller), error_msg);
- if (rpc_status.ok()) break;
- // TODO(IMPALA-8143) Add a sleep if RpcMgr::IsServerTooBusy().
- }
- return rpc_status;
- }
-
private:
/// Tracks the memory usage of payload in the service queue.
std::unique_ptr<MemTracker> mem_tracker_;
[impala] 01/07: Update mustache to commit
b290952d8eb93d085214d8c8c9eab8559df9f606
Posted by ta...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 25c8b6226ba90e0fee7bdd61ae5313570ce28754
Author: Todd Lipcon <to...@apache.org>
AuthorDate: Fri Jun 7 14:30:51 2019 -0700
Update mustache to commit b290952d8eb93d085214d8c8c9eab8559df9f606
This fixes some issues with regard to template variable scoping that
will be useful for Knox integration.
Change-Id: If26f3aaa2a3279a1f6a300c4f4cee7ec899e22ed
Reviewed-on: http://gerrit.cloudera.org:8080/13563
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/thirdparty/mustache/README | 2 +-
be/src/thirdparty/mustache/mustache.cc | 280 +++++++++++++++++++++------------
be/src/thirdparty/mustache/mustache.h | 2 +-
be/src/util/webserver.cc | 4 +-
4 files changed, 185 insertions(+), 103 deletions(-)
diff --git a/be/src/thirdparty/mustache/README b/be/src/thirdparty/mustache/README
index 9834079..ba94e4d 100644
--- a/be/src/thirdparty/mustache/README
+++ b/be/src/thirdparty/mustache/README
@@ -1,3 +1,3 @@
This Mustache template library is taken from the Apache licensed version at https://github.com/henryr/cpp-mustache
-The current commit is: 80cbe5
+The current commit is: b290952d8eb93d085214d8c8c9eab8559df9f606
diff --git a/be/src/thirdparty/mustache/mustache.cc b/be/src/thirdparty/mustache/mustache.cc
index fc6c4f6..e976677 100644
--- a/be/src/thirdparty/mustache/mustache.cc
+++ b/be/src/thirdparty/mustache/mustache.cc
@@ -13,14 +13,15 @@
#include "mustache.h"
#include "rapidjson/stringbuffer.h"
+#include <rapidjson/prettywriter.h>
#include "rapidjson/writer.h"
#include <iostream>
#include <fstream>
#include <vector>
+#include <stack>
#include <boost/algorithm/string.hpp>
-#include <boost/foreach.hpp>
using namespace rapidjson;
using namespace std;
@@ -30,7 +31,6 @@ namespace mustache {
// TODO:
// # Handle malformed templates better
-// # Support array_tag.length?
// # Better support for reading templates from files
enum TagOperator {
@@ -42,9 +42,24 @@ enum TagOperator {
PARTIAL,
COMMENT,
LENGTH,
+ EQUALITY,
+ INEQUALITY,
+ LITERAL,
NONE
};
+struct OpCtx {
+ TagOperator op;
+ string tag_name;
+ string tag_arg;
+ bool escaped = false;
+};
+
+struct ContextStack {
+ const Value* value;
+ const ContextStack* parent;
+};
+
TagOperator GetOperator(const string& tag) {
if (tag.size() == 0) return SUBSTITUTION;
switch (tag[0]) {
@@ -53,18 +68,24 @@ TagOperator GetOperator(const string& tag) {
case '?': return PREDICATE_SECTION_START;
case '/': return SECTION_END;
case '>': return PARTIAL;
- case '!': return COMMENT;
+ case '!':
+ if (tag.size() == 1 || tag[1] != '=') return COMMENT;
+ return INEQUALITY;
case '%': return LENGTH;
+ case '~': return LITERAL;
+ case '=': return EQUALITY;
default: return SUBSTITUTION;
}
}
int EvaluateTag(const string& document, const string& document_root, int idx,
- const Value* context, TagOperator tag, const string& tag_name, bool is_triple,
- stringstream* out);
+ const ContextStack* context, const OpCtx& op_ctx, stringstream* out);
+
+static bool RenderTemplate(const string& document, const string& document_root,
+ const ContextStack* stack, stringstream* out);
void EscapeHtml(const string& in, stringstream *out) {
- BOOST_FOREACH(const char& c, in) {
+ for (const char& c: in) {
switch (c) {
case '&': (*out) << "&";
break;
@@ -82,6 +103,13 @@ void EscapeHtml(const string& in, stringstream *out) {
}
}
+void Dump(const rapidjson::Value& v) {
+ StringBuffer buffer;
+ Writer<StringBuffer> writer(buffer);
+ v.Accept(writer);
+ std::cout << buffer.GetString() << std::endl;
+}
+
// Breaks a dotted path into individual components. One wrinkle, which stops this from
// being a simple split() is that we allow path components to be quoted, e.g.: "foo".bar,
// and any '.' characters inside those quoted sections aren't considered to be
@@ -122,38 +150,45 @@ void FindJsonPathComponents(const string& path, vector<string>* components) {
}
// Looks up the json entity at 'path' in 'parent_context', and places it in 'resolved'. If
-// the entity does not exist (i.e. the path is invalid), 'resolved' will be set to NULL.
-void ResolveJsonContext(const string& path, const Value& parent_context,
+// the entity does not exist (i.e. the path is invalid), 'resolved' will be set to nullptr.
+void ResolveJsonContext(const string& path, const ContextStack* stack,
const Value** resolved) {
if (path == ".") {
- *resolved = &parent_context;
+ *resolved = stack->value;
return;
}
vector<string> components;
FindJsonPathComponents(path, &components);
- const Value* cur = &parent_context;
- BOOST_FOREACH(const string& c, components) {
- if (cur->IsObject() && cur->HasMember(c.c_str())) {
- cur = &(*cur)[c.c_str()];
- } else {
- *resolved = NULL;
+
+ // At each enclosing level of context, try to resolve the path.
+ for ( ; stack != nullptr; stack = stack->parent) {
+ const Value* cur = stack->value;
+ bool match = true;
+ for(const string& c: components) {
+ if (cur->IsObject() && cur->HasMember(c.c_str())) {
+ cur = &(*cur)[c.c_str()];
+ } else {
+ match = false;
+ break;
+ }
+ }
+ if (match) {
+ *resolved = cur;
return;
}
}
-
- *resolved = cur;
+ *resolved = nullptr;
}
-int FindNextTag(const string& document, int idx, TagOperator* tag_op, string* tag_name,
- bool* is_triple, stringstream* out) {
- *tag_op = NONE;
+int FindNextTag(const string& document, int idx, OpCtx* op, stringstream* out) {
+ op->op = NONE;
while (idx < document.size()) {
if (document[idx] == '{' && idx < (document.size() - 3) && document[idx + 1] == '{') {
if (document[idx + 2] == '{') {
idx += 3;
- *is_triple = true;
+ op->escaped = true;
} else {
- *is_triple = false;
+ op->escaped = false;
idx += 2; // Now at start of template expression
}
stringstream expr;
@@ -162,10 +197,10 @@ int FindNextTag(const string& document, int idx, TagOperator* tag_op, string* ta
expr << document[idx];
++idx;
} else {
- if (!*is_triple && idx < document.size() - 1 && document[idx + 1] == '}') {
+ if (!op->escaped && idx < document.size() - 1 && document[idx + 1] == '}') {
++idx;
break;
- } else if (*is_triple && idx < document.size() - 2 && document[idx + 1] == '}'
+ } else if (op->escaped && idx < document.size() - 2 && document[idx + 1] == '}'
&& document[idx + 2] == '}') {
idx += 2;
break;
@@ -179,16 +214,27 @@ int FindNextTag(const string& document, int idx, TagOperator* tag_op, string* ta
trim(key);
if (key != ".") trim_if(key, is_any_of("."));
if (key.size() == 0) continue;
- *tag_op = GetOperator(key);
- if (*tag_op != SUBSTITUTION) {
- key = key.substr(1);
+ op->op = GetOperator(key);
+ if (op->op != SUBSTITUTION) {
+ int len = op->op == INEQUALITY ? 2 : 1;
+ key = key.substr(len);
trim(key);
}
if (key.size() == 0) continue;
- *tag_name = key;
+
+ if (op->op == EQUALITY || op->op == INEQUALITY) {
+ // Find an argument
+ vector<string> components;
+ split(components, key, is_any_of(" "));
+ key = components[0];
+ components.erase(components.begin());
+ op->tag_arg = join(components, " ");
+ }
+
+ op->tag_name = key;
return ++idx;
} else {
- if (out != NULL) (*out) << document[idx];
+ if (out != nullptr) (*out) << document[idx];
}
++idx;
}
@@ -203,57 +249,69 @@ int FindNextTag(const string& document, int idx, TagOperator* tag_op, string* ta
// If 'is_negation' is true, the behaviour is the opposite of the above: false values
// cause the section to be normally evaluated etc.
int EvaluateSection(const string& document, const string& document_root, int idx,
- const Value* parent_context, TagOperator op, const string& tag_name,
- stringstream* out) {
- // Precondition: idx is the immedate next character after an opening {{ #tag_name }}
+ const ContextStack* context_stack, const OpCtx& op_ctx, stringstream* out) {
+ // Precondition: idx is the immediate next character after an opening {{ #tag_name }}
const Value* context;
- ResolveJsonContext(tag_name, *parent_context, &context);
+ ResolveJsonContext(op_ctx.tag_name, context_stack, &context);
// If we a) cannot resolve the context from the tag name or b) the context evaluates to
// false, we should skip the contents of the template until a closing {{/tag_name}}.
- bool skip_contents = (context == NULL || context->IsFalse());
-
- // If the tag is a negative block (i.e. {{^tag_name}}), do the opposite: if the context
- // exists and is true, skip the contents, else echo them.
- if (op == NEGATED_SECTION_START) {
- context = parent_context;
- skip_contents = !skip_contents;
- } else if (op == PREDICATE_SECTION_START) {
- context = parent_context;
+ bool skip_contents = false;
+
+ if (op_ctx.op == NEGATED_SECTION_START || op_ctx.op == PREDICATE_SECTION_START ||
+ op_ctx.op == SECTION_START) {
+ skip_contents = (context == nullptr || context->IsFalse());
+
+ // If the tag is a negative block (i.e. {{^tag_name}}), do the opposite: if the
+ // context exists and is true, skip the contents, else echo them.
+ if (op_ctx.op == NEGATED_SECTION_START) {
+ context = context_stack->value;
+ skip_contents = !skip_contents;
+ } else if (op_ctx.op == PREDICATE_SECTION_START) {
+ context = context_stack->value;
+ }
+ } else if (op_ctx.op == INEQUALITY || op_ctx.op == EQUALITY) {
+ skip_contents = (context == nullptr || !context->IsString() ||
+ strcasecmp(context->GetString(), op_ctx.tag_arg.c_str()) != 0);
+ if (op_ctx.op == INEQUALITY) skip_contents = !skip_contents;
+ context = context_stack->value;
}
vector<const Value*> values;
- if (!skip_contents && context != NULL && context->IsArray()) {
+ if (!skip_contents && context != nullptr && context->IsArray()) {
for (int i = 0; i < context->Size(); ++i) {
values.push_back(&(*context)[i]);
}
} else {
- values.push_back(skip_contents ? NULL : context);
+ values.push_back(skip_contents ? nullptr : context);
}
if (values.size() == 0) {
skip_contents = true;
- values.push_back(NULL);
+ values.push_back(nullptr);
}
int start_idx = idx;
- BOOST_FOREACH(const Value* v, values) {
+ for(const Value* v: values) {
idx = start_idx;
+ stack<OpCtx> section_starts;
+ section_starts.push(op_ctx);
while (idx < document.size()) {
- TagOperator tag_op;
- string next_tag_name;
- bool is_triple;
- idx = FindNextTag(document, idx, &tag_op, &next_tag_name, &is_triple,
- skip_contents ? NULL : out);
-
- if (idx > document.size()) return idx;
- if (tag_op == SECTION_END && next_tag_name == tag_name) {
- break;
+ OpCtx next_ctx;
+ idx = FindNextTag(document, idx, &next_ctx, skip_contents ? nullptr : out);
+ if (skip_contents && (next_ctx.op == SECTION_START ||
+ next_ctx.op == PREDICATE_SECTION_START ||
+ next_ctx.op == NEGATED_SECTION_START)) {
+ section_starts.push(next_ctx);
+ } else if (next_ctx.op == SECTION_END) {
+ if (next_ctx.tag_name != section_starts.top().tag_name) return -1;
+ section_starts.pop();
}
+ if (section_starts.empty()) break;
// Don't need to evaluate any templates if we're skipping the contents
if (!skip_contents) {
- idx = EvaluateTag(document, document_root, idx, v, tag_op, next_tag_name,
- is_triple, out);
+ ContextStack new_context = { v, context_stack };
+ idx = EvaluateTag(document, document_root, idx, &new_context, next_ctx, out);
}
}
}
@@ -263,53 +321,65 @@ int EvaluateSection(const string& document, const string& document_root, int idx
// Evaluates a SUBSTITUTION tag, by replacing its contents with the value of the tag's
// name in 'parent_context'.
int EvaluateSubstitution(const string& document, const int idx,
- const Value* parent_context, const string& tag_name, bool is_triple,
- stringstream* out) {
- const Value* context;
- ResolveJsonContext(tag_name, *parent_context, &context);
- if (context == NULL) return idx;
- if (context->IsString()) {
- if (!is_triple) {
- EscapeHtml(context->GetString(), out);
+ const ContextStack* context_stack, const OpCtx& op_ctx, stringstream* out) {
+ const Value* val;
+ ResolveJsonContext(op_ctx.tag_name, context_stack, &val);
+ if (val == nullptr) return idx;
+ if (val->IsString()) {
+ if (!op_ctx.escaped) {
+ EscapeHtml(val->GetString(), out);
} else {
// TODO: Triple {{{ means don't escape
- (*out) << context->GetString();
+ (*out) << val->GetString();
}
- } else if (context->IsInt64()) {
- (*out) << context->GetInt64();
- } else if (context->IsInt()) {
- (*out) << context->GetInt();
- } else if (context->IsDouble()) {
- (*out) << context->GetDouble();
- } else if (context->IsBool()) {
- (*out) << boolalpha << context->GetBool();
+ } else if (val->IsInt64()) {
+ (*out) << val->GetInt64();
+ } else if (val->IsInt()) {
+ (*out) << val->GetInt();
+ } else if (val->IsDouble()) {
+ (*out) << val->GetDouble();
+ } else if (val->IsBool()) {
+ (*out) << boolalpha << val->GetBool();
}
return idx;
}
// Evaluates a LENGTH tag by replacing its contents with the type-dependent 'size' of the
// value.
-int EvaluateLength(const string& document, const int idx, const Value* parent_context,
+int EvaluateLength(const string& document, const int idx, const ContextStack* context_stack,
const string& tag_name, stringstream* out) {
- const Value* context;
- ResolveJsonContext(tag_name, *parent_context, &context);
- if (context == NULL) return idx;
- if (context->IsArray()) {
- (*out) << context->Size();
- } else if (context->IsString()) {
- (*out) << context->GetStringLength();
+ const Value* val;
+ ResolveJsonContext(tag_name, context_stack, &val);
+ if (val == nullptr) return idx;
+ if (val->IsArray()) {
+ (*out) << val->Size();
+ } else if (val->IsString()) {
+ (*out) << val->GetStringLength();
};
return idx;
}
+int EvaluateLiteral(const string& document, const int idx, const ContextStack* context_stack,
+ const string& tag_name, stringstream* out) {
+ const Value* val;
+ ResolveJsonContext(tag_name, context_stack, &val);
+ if (val == nullptr) return idx;
+ if (!val->IsArray() && !val->IsObject()) return idx;
+ StringBuffer strbuf;
+ PrettyWriter<StringBuffer> writer(strbuf);
+ val->Accept(writer);
+ (*out) << strbuf.GetString();
+ return idx;
+}
+
// Evaluates a 'partial' template by reading it fully from disk, then rendering it
// directly into the current output with the current context.
//
// TODO: This could obviously be more efficient (and there are lots of file accesses in a
// long list context).
void EvaluatePartial(const string& tag_name, const string& document_root,
- const Value* parent_context, stringstream* out) {
+ const ContextStack* stack, stringstream* out) {
stringstream ss;
ss << document_root << tag_name;
ifstream tmpl(ss.str().c_str());
@@ -320,49 +390,59 @@ void EvaluatePartial(const string& tag_name, const string& document_root,
}
stringstream file_ss;
file_ss << tmpl.rdbuf();
- RenderTemplate(file_ss.str(), document_root, *parent_context, out);
+ RenderTemplate(file_ss.str(), document_root, stack, out);
}
// Given a tag name, and its operator, evaluate the tag in the given context and write the
// output to 'out'. The heavy-lifting is delegated to specific Evaluate*()
// methods. Returns the new cursor position within 'document', or -1 on error.
int EvaluateTag(const string& document, const string& document_root, int idx,
- const Value* context, TagOperator tag,
- const string& tag_name, bool is_triple, stringstream* out) {
+ const ContextStack* context, const OpCtx& op_ctx, stringstream* out) {
if (idx == -1) return idx;
- switch (tag) {
+ switch (op_ctx.op) {
case SECTION_START:
case PREDICATE_SECTION_START:
case NEGATED_SECTION_START:
- return EvaluateSection(document, document_root, idx, context, tag, tag_name, out);
+ case EQUALITY:
+ case INEQUALITY:
+ return EvaluateSection(document, document_root, idx, context, op_ctx, out);
case SUBSTITUTION:
- return EvaluateSubstitution(document, idx, context, tag_name, is_triple, out);
+ return EvaluateSubstitution(document, idx, context, op_ctx, out);
case COMMENT:
return idx; // Ignored
case PARTIAL:
- EvaluatePartial(tag_name, document_root, context, out);
+ EvaluatePartial(op_ctx.tag_name, document_root, context, out);
return idx;
case LENGTH:
- return EvaluateLength(document, idx, context, tag_name, out);
+ return EvaluateLength(document, idx, context, op_ctx.tag_name, out);
+ case LITERAL:
+ return EvaluateLiteral(document, idx, context, op_ctx.tag_name, out);
case NONE:
return idx; // No tag was found
+ case SECTION_END:
+ return idx;
default:
- cout << "Unknown tag: " << tag << endl;
+ cout << "Unknown tag: " << op_ctx.op << endl;
return -1;
}
}
-void RenderTemplate(const string& document, const string& document_root,
- const Value& context, stringstream* out) {
+static bool RenderTemplate(const string& document, const string& document_root,
+ const ContextStack* stack, stringstream* out) {
int idx = 0;
while (idx < document.size() && idx != -1) {
- string tag_name;
- TagOperator tag_op;
- bool is_triple = true;
- idx = FindNextTag(document, idx, &tag_op, &tag_name, &is_triple, out);
- idx = EvaluateTag(document, document_root, idx, &context, tag_op, tag_name, is_triple,
- out);
+ OpCtx op;
+ idx = FindNextTag(document, idx, &op, out);
+ idx = EvaluateTag(document, document_root, idx, stack, op, out);
}
+
+ return idx != -1;
+}
+
+bool RenderTemplate(const string& document, const string& document_root,
+ const Value& context, stringstream* out) {
+ ContextStack stack = { &context, nullptr };
+ return RenderTemplate(document, document_root, &stack, out);
}
}
diff --git a/be/src/thirdparty/mustache/mustache.h b/be/src/thirdparty/mustache/mustache.h
index e4168d3..9a2d9fb 100644
--- a/be/src/thirdparty/mustache/mustache.h
+++ b/be/src/thirdparty/mustache/mustache.h
@@ -21,7 +21,7 @@ namespace mustache {
// 'context'. Alternately finds a tag and then evaluates it. Returns when an error is
// signalled (TODO: probably doesn't work in all paths), and evaluates that tag. Output is
// accumulated in 'out'.
-void RenderTemplate(const std::string& document, const std::string& document_root,
+bool RenderTemplate(const std::string& document, const std::string& document_root,
const rapidjson::Value& context, std::stringstream* out);
}
diff --git a/be/src/util/webserver.cc b/be/src/util/webserver.cc
index 331c6d9..3ea3e65 100644
--- a/be/src/util/webserver.cc
+++ b/be/src/util/webserver.cc
@@ -541,8 +541,10 @@ void Webserver::RenderUrlWithTemplate(const WebRequest& req,
} else {
stringstream buffer;
buffer << tmpl.rdbuf();
- RenderTemplate(buffer.str(), Substitute("$0/", FLAGS_webserver_doc_root), document,
+ bool success = RenderTemplate(buffer.str(),
+ Substitute("$0/", FLAGS_webserver_doc_root), document,
output);
+ LOG_IF(WARNING, !success) << "could not render template " << full_template_path;
}
}
}