You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/01/07 17:05:52 UTC
[GitHub] [hive] klcopp commented on a change in pull request #1839: HIVE-24602: Retry compaction after configured time
klcopp commented on a change in pull request #1839:
URL: https://github.com/apache/hive/pull/1839#discussion_r553455629
##########
File path: ql/src/test/org/apache/hadoop/hive/metastore/txn/TestCompactionTxnHandler.java
##########
@@ -289,11 +290,15 @@ public void testMarkFailed() throws Exception {
assertFalse(txnHandler.checkFailedCompactions(ci));
// Add more failed compactions so that the total is exactly COMPACTOR_INITIATOR_FAILED_THRESHOLD
- for (int i = 1 ; i < conf.getIntVar(HiveConf.ConfVars.COMPACTOR_INITIATOR_FAILED_THRESHOLD); i++) {
+ for (int i = 1; i < MetastoreConf.getIntVar(conf, MetastoreConf.ConfVars.COMPACTOR_INITIATOR_FAILED_THRESHOLD); i++) {
addFailedCompaction(dbName, tableName, CompactionType.MINOR, partitionName, errorMessage);
}
// Now checkFailedCompactions() will return true
assertTrue(txnHandler.checkFailedCompactions(ci));
+ MetastoreConf.setTimeVar(conf, MetastoreConf.ConfVars.COMPACTOR_INITIATOR_FAILED_RETRY_TIME, 1, TimeUnit.MILLISECONDS);
+ // Now checkFailedCompactions() should ignore the treshold
Review comment:
nit: typo in threshold
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -1006,16 +1007,23 @@ public boolean checkFailedCompactions(CompactionInfo ci) throws MetaException {
rs = pStmt.executeQuery();
int numFailed = 0;
int numTotal = 0;
+ long lastEnqueueTime = -1;
int failedThreshold = MetastoreConf.getIntVar(conf, ConfVars.COMPACTOR_INITIATOR_FAILED_THRESHOLD);
while(rs.next() && ++numTotal <= failedThreshold) {
+ long enqueueTime = rs.getLong(2);
+ if (enqueueTime > lastEnqueueTime) {
+ lastEnqueueTime = enqueueTime;
+ }
if(rs.getString(1).charAt(0) == FAILED_STATE) {
numFailed++;
}
else {
numFailed--;
}
}
- return numFailed == failedThreshold;
+ // If the last attempt was too long ago, ignore the failed treshold and try compaction again
Review comment:
nit: typo in threshold
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -1006,16 +1007,23 @@ public boolean checkFailedCompactions(CompactionInfo ci) throws MetaException {
rs = pStmt.executeQuery();
int numFailed = 0;
int numTotal = 0;
+ long lastEnqueueTime = -1;
int failedThreshold = MetastoreConf.getIntVar(conf, ConfVars.COMPACTOR_INITIATOR_FAILED_THRESHOLD);
while(rs.next() && ++numTotal <= failedThreshold) {
+ long enqueueTime = rs.getLong(2);
+ if (enqueueTime > lastEnqueueTime) {
+ lastEnqueueTime = enqueueTime;
+ }
if(rs.getString(1).charAt(0) == FAILED_STATE) {
numFailed++;
}
else {
numFailed--;
}
}
- return numFailed == failedThreshold;
+ // If the last attempt was too long ago, ignore the failed treshold and try compaction again
+ long retryTime = MetastoreConf.getTimeVar(conf, ConfVars.COMPACTOR_INITIATOR_FAILED_RETRY_TIME, TimeUnit.MILLISECONDS);
Review comment:
The config's value is retrieved every time each table/partition is checked. Might be better to get it in the main loop of Initiator instead.
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
##########
@@ -415,6 +415,10 @@ public static ConfVars getMetaConf(String name) {
new RangeValidator(1, 20), "Number of consecutive compaction failures (per table/partition) " +
"after which automatic compactions will not be scheduled any more. Note that this must be less " +
"than hive.compactor.history.retention.failed."),
+ COMPACTOR_INITIATOR_FAILED_RETRY_TIME("metastore.compactor.initiator.failed.retry.time",
Review comment:
might want to include that this config is disabled if set to a negative value
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
##########
@@ -415,6 +415,10 @@ public static ConfVars getMetaConf(String name) {
new RangeValidator(1, 20), "Number of consecutive compaction failures (per table/partition) " +
"after which automatic compactions will not be scheduled any more. Note that this must be less " +
"than hive.compactor.history.retention.failed."),
+ COMPACTOR_INITIATOR_FAILED_RETRY_TIME("metastore.compactor.initiator.failed.retry.time",
+ "hive.compactor.initiator.failed.retry.time", 7, TimeUnit.DAYS,
+ "Time after Initiator will ignore metastore.compactor.initiator.failed.compacts.threshold and retry with compaction again. \n"
Review comment:
nit: lines should be max 120 characters in length
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java
##########
@@ -993,7 +994,7 @@ public boolean checkFailedCompactions(CompactionInfo ci) throws MetaException {
try {
try {
dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED);
- pStmt = dbConn.prepareStatement("SELECT \"CC_STATE\" FROM \"COMPLETED_COMPACTIONS\" WHERE " +
+ pStmt = dbConn.prepareStatement("SELECT \"CC_STATE\", \"CC_ENQUEUE_TIME\" FROM \"COMPLETED_COMPACTIONS\" WHERE " +
Review comment:
Is there a reason for using CC_ENQUEUE_TIME over CC_END (or CC_START)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org