klcopp commented on code in PR #3513:
URL: https://github.com/apache/hive/pull/3513#discussion_r949159305
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -14214,7 +14214,7 @@ private void validateCreateView()
if (SemanticAnalyzer.DUMMY_TABLE.equals(table.getTableName())) {
continue;
}
- if (!AcidUtils.isTransactionalTable(table)) {
+ if (!AcidUtils.isTransactionalTable(table) &&
!AcidUtils.isNonNativeAcidTable(table)) {
Review Comment:
What does this do?
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -2966,7 +2971,7 @@ PartitionsResponse get_partitions_req(1:PartitionsRequest
req)
bool submit_for_cleanup(1:CompactionRequest o1, 2:i64 o2, 3:i64 o3) throws
(1:MetaException o1)
void add_dynamic_partitions(1:AddDynamicPartitions rqst) throws
(1:NoSuchTxnException o1, 2:TxnAbortedException o2)
// Deprecated, use find_next_compact2()
- OptionalCompactionInfoStruct find_next_compact(1: string workerId)
throws(1:MetaException o1)
+ OptionalCompactionInfoStruct find_next_compact(1: string workerId, 2: string
poolName) throws(1:MetaException o1)
Review Comment:
Hm, I'm not sure I understand why we need this change if we can use
find_next_compact2?
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -441,6 +441,12 @@ public enum ConfVars {
COMPACTOR_USE_CUSTOM_POOL("metastore.compactor.use.custom.pool",
"hive.compactor.use.custom.pool",
false, "internal usage only -- use custom connection pool specific
to compactor components."
),
+ COMPACTOR_WORKER_POOL_TIMEOUT(
+ "metastore.compactor.worker.pool.timeout",
+ "hive.compactor.worker.pool.timeout",
+ 12, TimeUnit.HOURS,
+ "Time in seconds after which a compaction job will be declared failed
and the\n" +
Review Comment:
Also I think a better description would be something like:
Time in hours after which a compaction assigned to a custom compaction
Worker pool can be picked up by the default compaction Worker pool. (Is that
correct?)
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -229,17 +231,33 @@ public CompactionInfo
findNextToCompact(FindNextCompactRequest rqst) throws Meta
}
Connection dbConn = null;
- Statement stmt = null;
+ PreparedStatement stmt = null;
//need a separate stmt for executeUpdate() otherwise it will close the
ResultSet(HIVE-12725)
Statement updStmt = null;
ResultSet rs = null;
+
+ long poolTimeout = MetastoreConf.getTimeVar(conf,
ConfVars.COMPACTOR_WORKER_POOL_TIMEOUT, TimeUnit.MILLISECONDS);
+
try {
dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED,
connPoolCompaction);
- stmt = dbConn.createStatement();
- String query = "SELECT \"CQ_ID\", \"CQ_DATABASE\", \"CQ_TABLE\",
\"CQ_PARTITION\", " +
- "\"CQ_TYPE\", \"CQ_TBLPROPERTIES\" FROM \"COMPACTION_QUEUE\" WHERE
\"CQ_STATE\" = '" + INITIATED_STATE + "'";
+ StringBuilder sb = new StringBuilder();
+ sb.append("SELECT \"CQ_ID\", \"CQ_DATABASE\", \"CQ_TABLE\",
\"CQ_PARTITION\", " +
+ "\"CQ_TYPE\", \"CQ_POOL_NAME\", \"CQ_TBLPROPERTIES\" FROM
\"COMPACTION_QUEUE\" WHERE \"CQ_STATE\" = '" + INITIATED_STATE + "' AND ");
+ boolean hasPoolName =
org.apache.commons.lang3.StringUtils.isNotBlank(rqst.getPoolName());
+ if(hasPoolName) {
+ sb.append("\"CQ_POOL_NAME\"=?");
+ } else {
+ sb.append("\"CQ_POOL_NAME\" is null OR \"CQ_ENQUEUE_TIME\" < (")
Review Comment:
(nit: it would be nicer if "is null" were all caps)
##########
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java:
##########
@@ -3205,6 +3205,9 @@ public static enum ConfVars {
"Time in seconds after which a compaction job will be declared failed
and the\n" +
"compaction re-queued."),
+ HIVE_COMPACTOR_WORKER_POOL("hive.compactor.worker.pool", "",
Review Comment:
You can just put this in Constants.java since this is not a hive
session/server config but a tblproperty name
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/txn/compactor/TestCompactor.java:
##########
@@ -17,40 +17,13 @@
*/
package org.apache.hadoop.hive.ql.txn.compactor;
-import static org.apache.hadoop.hive.common.AcidConstants.VISIBILITY_PATTERN;
Review Comment:
FYI refactoring is harder to backport to other branches, but since this is a
new feature (i.e. not a bug fix) you should be safe from needing to backport
this to older branches. So this is probably fine, just a heads up
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -441,6 +441,12 @@ public enum ConfVars {
COMPACTOR_USE_CUSTOM_POOL("metastore.compactor.use.custom.pool",
"hive.compactor.use.custom.pool",
false, "internal usage only -- use custom connection pool specific
to compactor components."
),
+ COMPACTOR_WORKER_POOL_TIMEOUT(
+ "metastore.compactor.worker.pool.timeout",
+ "hive.compactor.worker.pool.timeout",
+ 12, TimeUnit.HOURS,
+ "Time in seconds after which a compaction job will be declared failed
and the\n" +
Review Comment:
Should be "time in hours."
(Users can set to e.g. "30m" for minutes or "10s" for 10 seconds, but if
they input "3" then that will mean hours since the default time unit set here
is TimeUnit.HOURS)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
