You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by "veghlaci05 (via GitHub)" <gi...@apache.org> on 2023/05/16 12:17:30 UTC
[GitHub] [hive] veghlaci05 commented on a diff in pull request #4313: HIVE-27332: Add retry backoff mechanism for abort cleanup
veghlaci05 commented on code in PR #4313:
URL: https://github.com/apache/hive/pull/4313#discussion_r1194867693
##########
ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/TestCleaner.java:
##########
@@ -811,6 +812,7 @@ boolean useHive130DeltaDirName() {
}
@After
+ @AfterEach
Review Comment:
Is it necessary to have both Junit 4 and 5 annotations at the same time?
##########
ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/CompactorTest.java:
##########
@@ -129,6 +130,7 @@ public abstract class CompactorTest {
FileSystem fs;
@Before
+ @BeforeEach
Review Comment:
Is it necessary to have both Junit 4 and 5 annotations at the same time?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -1562,16 +1620,46 @@ public void markRefused(CompactionInfo info) throws MetaException {
@Override
@RetrySemantics.CannotRetry
- public void setCleanerRetryRetentionTimeOnError(CompactionInfo info) throws MetaException {
+ public void setCleanerRetryRetentionTimeOnError(CompactionInfo info, boolean isAbort) throws MetaException {
Review Comment:
I would create a separate class for Abort requests instead of reusing CompactionInfo. Instead of having an entity which unions all the fields which may required in certain scenarios, I would prefer having dedicated entities for different use cases. Currently CompactionInfo used everywhare, but in most of the cases (just like here) only a few of its fields are filled. This requires extra caution to not try to read a field which is there but has no value.
I would also separate the retry logic into separate methods as they are completely independent from each other. The need of the new isAbort param is a good sign of this: You are using the same object, but the behavior is completely different.
@deniskuzZ WDYT?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -1562,16 +1620,46 @@ public void markRefused(CompactionInfo info) throws MetaException {
@Override
@RetrySemantics.CannotRetry
- public void setCleanerRetryRetentionTimeOnError(CompactionInfo info) throws MetaException {
+ public void setCleanerRetryRetentionTimeOnError(CompactionInfo info, boolean isAbort) throws MetaException {
try {
try (Connection dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED, connPoolCompaction)) {
- try (PreparedStatement stmt = dbConn.prepareStatement("UPDATE \"COMPACTION_QUEUE\" " +
- "SET \"CQ_RETRY_RETENTION\" = ?, \"CQ_ERROR_MESSAGE\"= ? WHERE \"CQ_ID\" = ?")) {
+ String query;
+ if (isAbort) {
+ // Check whether we need to do an insert to the TXN_CLEANUP_QUEUE or an update to TXN_CLEANUP_QUEUE.
+ if (!info.hasOldAbort) {
+ if (info.partName != null) {
+ query = "INSERT INTO \"TXN_CLEANUP_QUEUE\" (\"TCQ_RETRY_RETENTION\", \"TCQ_ERROR_MESSAGE\", " +
+ "\"TCQ_DATABASE\", \"TCQ_TABLE\", \"TCQ_PARTITION\", \"TCQ_RETRY_TIME\") VALUES (?, ?, ?, ?, ?, " + getEpochFn(dbProduct) + ")";
+ } else {
+ query = "INSERT INTO \"TXN_CLEANUP_QUEUE\" (\"TCQ_RETRY_RETENTION\", \"TCQ_ERROR_MESSAGE\", " +
+ "\"TCQ_DATABASE\", \"TCQ_TABLE\", \"TCQ_RETRY_TIME\") VALUES (?, ?, ?, ?, " + getEpochFn(dbProduct) + ")";
+ }
+ } else {
+ query = "UPDATE \"TXN_CLEANUP_QUEUE\" SET \"TCQ_RETRY_RETENTION\" = ?, \"TCQ_ERROR_MESSAGE\" = ? " +
+ "WHERE \"TCQ_DATABASE\" = ? AND \"TCQ_TABLE\" = ? AND \"TCQ_PARTITION\" " + (info.partName != null ? "= ?" : "IS NULL");
Review Comment:
How about sth like
`... ( "TCQ_PARTITION" = ? OR "TCQ_PARTITION" IS NULL)`
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -772,6 +804,32 @@ private void removeTxnComponents(Connection dbConn, CompactionInfo info) throws
}
}
+ private void removeRetryQueueEntries(Connection dbConn, CompactionInfo info) throws MetaException, RetryException {
+ PreparedStatement pStmt = null;
+ String query = "DELETE FROM \"TXN_CLEANUP_QUEUE\" WHERE \"TCQ_DATABASE\" = ? " +
+ "AND \"TCQ_TABLE\" = ? AND \"TCQ_PARTITION\" " + (info.partName != null ? "= ?" : "IS NULL");
Review Comment:
How about sth like
`... ( "TCQ_PARTITION" = ? OR "TCQ_PARTITION" IS NULL)`
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/CompactionTxnHandler.java:
##########
@@ -1562,16 +1620,46 @@ public void markRefused(CompactionInfo info) throws MetaException {
@Override
@RetrySemantics.CannotRetry
- public void setCleanerRetryRetentionTimeOnError(CompactionInfo info) throws MetaException {
+ public void setCleanerRetryRetentionTimeOnError(CompactionInfo info, boolean isAbort) throws MetaException {
try {
try (Connection dbConn = getDbConn(Connection.TRANSACTION_READ_COMMITTED, connPoolCompaction)) {
- try (PreparedStatement stmt = dbConn.prepareStatement("UPDATE \"COMPACTION_QUEUE\" " +
- "SET \"CQ_RETRY_RETENTION\" = ?, \"CQ_ERROR_MESSAGE\"= ? WHERE \"CQ_ID\" = ?")) {
+ String query;
+ if (isAbort) {
+ // Check whether we need to do an insert to the TXN_CLEANUP_QUEUE or an update to TXN_CLEANUP_QUEUE.
+ if (!info.hasOldAbort) {
+ if (info.partName != null) {
+ query = "INSERT INTO \"TXN_CLEANUP_QUEUE\" (\"TCQ_RETRY_RETENTION\", \"TCQ_ERROR_MESSAGE\", " +
+ "\"TCQ_DATABASE\", \"TCQ_TABLE\", \"TCQ_PARTITION\", \"TCQ_RETRY_TIME\") VALUES (?, ?, ?, ?, ?, " + getEpochFn(dbProduct) + ")";
+ } else {
+ query = "INSERT INTO \"TXN_CLEANUP_QUEUE\" (\"TCQ_RETRY_RETENTION\", \"TCQ_ERROR_MESSAGE\", " +
+ "\"TCQ_DATABASE\", \"TCQ_TABLE\", \"TCQ_RETRY_TIME\") VALUES (?, ?, ?, ?, " + getEpochFn(dbProduct) + ")";
Review Comment:
Is it possible to merge these two branches and use `stmt.setNull(int, int)` when partition is null?
##########
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-4.0.0-alpha-2-to-4.0.0.derby.sql:
##########
@@ -26,5 +26,15 @@ CREATE INDEX "APP"."TAB_COL_STATS_IDX" ON "APP"."TAB_COL_STATS" ("DB_NAME", "TAB
DROP INDEX "APP"."PCS_STATS_IDX";
CREATE INDEX "APP"."PCS_STATS_IDX" ON "APP"."PART_COL_STATS" ("DB_NAME","TABLE_NAME","COLUMN_NAME","PARTITION_NAME","CAT_NAME");
+-- HIVE-27332
+CREATE TABLE TXN_CLEANUP_QUEUE (
+ TCQ_DATABASE varchar(128) NOT NULL,
+ TCQ_TABLE varchar(256) NOT NULL,
+ TCQ_PARTITION varchar(767),
Review Comment:
Just curious: how this value is calculated? :)
--
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: gitbox-unsubscribe@hive.apache.org
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