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: [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]
