HIVE-11495: Add aborted reason to transaction information #3908

SourabhBadhya · 2023-01-04T06:37:53Z

What changes were proposed in this pull request?

Add aborted reason to transaction information

Why are the changes needed?

For better debuggability and understanding of why a transaction is aborted.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Most of the aborted reason messages are logging, hence was able to check that the appropriate messages are logged through existing tests.

veghlaci05

The ticket says:

"Should add TXNS.COMMENT field or something..."

According to my understanding the abort reason should be presisted to the metastore, and should be shown using the show transactions command, or at least should be present in the TRANSACTIONS view

This part is completely missing from the PR. Do I missunderstand sth?

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java

veghlaci05 · 2023-01-04T11:00:46Z

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java

+        return errorCode;
+    }
+
+    public String getTxnErrorMsg() {


getTxnErrorMsg is not used at all. Where the error code translates to error message? How the users will know which code means what?

I think I will remove this function. The mapping of errorCode to TxnErrorMsg is done here -
https://github.com/apache/hive/pull/3908/files#diff-22cbee1c93bb4d0cedd41531cd2d8631483ec310a6727bbdb2d73f8c830b6d95R74-R81

This link redirects to iteslf for some reason but I guess you wanted to point to line 1099 in TxnHandler. My main concern about the messages what I have also added as a general comment: The messages are not persisted and not shown to the users in any way (except in the logs).

veghlaci05 · 2023-01-04T11:12:23Z

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java

+/**
+ * The following class represents all the error messages that are handled for aborts.
+ */
+public enum TxnErrorMsg {


Some messages are not real reasons, rather indicate only the type of the aborted txn. Please try to add a real reason as well, whenever possible.

All these messages are written based on where abortTxn()/abortTxns() is called. This represents the totality of how transactions are aborted from HiveServer. Anything else has not come from Hiveserver but rather from other services which might use transactional features of Hive (I have tried to represent them here as NONE). These are the only reasons where abort is done from Hiveserver.

SourabhBadhya · 2023-01-04T11:57:33Z

@veghlaci05 Based on a earlier conversation for the same ticket - #3656 (comment)
It was decided that we must opt for logging instead of persisting it. I have edited the same in the Jira as well.

veghlaci05 · 2023-01-04T12:38:47Z

@veghlaci05 Based on a earlier conversation for the same ticket - #3656 (comment) It was decided that we must opt for logging instead of persisting it. I have edited the same in the Jira as well.

In this please ignore my comments regarding the presistence. While I still think persisting would be better, I have to admit it would make this task significantly more complicated. (As @deniskuzZ pointed out, aborted TXNs are cleaned up quickly).

veghlaci05

LGTM, pending tests

@OverRide

* Source Iceberg PR - Core: Remove deprecated APIs up to 0.13.0 * Revert "HIVE-25563: Iceberg table operations hang a long time if metadata is missing/corrupted (Adam Szita, reviewed by Marton Bod)" - applying instead Hive: Limit number of retries when metadata file is missing (apache#3379) This reverts commit 7b600fe. * Source Iceberg PR - Hive: Limit number of retries when metadata file is missing (apache#3379) * Source Iceberg PR - Hive: Fix RetryingMetaStoreClient for Hive 2.1 (apache#3403) * Source Iceberg PR - Switch from new HashMap to Maps.newHashMap (apache#3648) * Source Iceberg PR - Hive: HiveCatalog should remove HMS stats for certain engines based on config (apache#3652) - Use the Iceberg config property * Source Iceberg PR - Core: If status check fails, commit should be unknown (apache#3717) * Source Iceberg PR - Build: Add checkstyle rule for instantiating HashMap, HashSet, ArrayList (apache#3689) * Source Iceberg PR - Test: Make sure to delete temp folders (apache#3790) * Source Iceberg PR - API: Register existing tables in Iceberg HiveCatalog (apache#3851) * Source Iceberg PR - Hive: Make Iceberg table filter optional in HiveCatalog (apache#3908) * Source Iceberg PR - Core: Add reserved UUID Table Property and Expose in HMS. (apache#3914) * Source Iceberg PR - Hive: Known exception should not become CommitStateUnknownException (apache#4261) * Source Iceberg PR - Build: Add missing @OverRide annotations (apache#3654)

deniskuzZ · 2023-01-05T10:10:25Z

...astore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java

+   * @param errorCode Error code specifying the reason for abort.
+   * @throws TException
+   */
+  void abortTxns(List<Long> txnids, long errorCode) throws TException;


we should make this method generic and use Request object here as well, see

CompactionResponse compact2(CompactionRequest request) void dropDatabase(DropDatabaseRequest req)

etc

Implemented. Done.

deniskuzZ · 2023-01-05T10:18:39Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

+        } else if (isHiveReplTxn) {
+          errorMsg = TxnErrorMsg.ABORT_DEFAULT_REPL_TXN;
+        } else {
+          errorMsg = TxnErrorMsg.ABORT_ROLLBACK;


why don't you set it in the var declaration?

Assigned TxnErrorMsg.NONE as default. Done.

deniskuzZ · 2023-01-05T10:20:29Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

  @RetrySemantics.Idempotent
  public void abortTxns(AbortTxnsRequest rqst) throws MetaException {
    List<Long> txnIds = rqst.getTxn_ids();
+    TxnErrorMsg txnErrorMsg = TxnErrorMsg.NONE;


in the above method (abortTxn(AbortTxnRequest rqst)), default value was ABORT_ROLLBACK and here it's NONE. Is this expected?

Changed default values in all places to TxnErrorMsg.NONE. Done.

deniskuzZ · 2023-01-05T10:26:36Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

    if (txnids.isEmpty()) {
      return 0;
    }
+    LOG.info("Trying to abort txns due to : {}", errorMsg);


maybe debug here?

deniskuzZ · 2023-01-05T10:27:49Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

      if (MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
        Metrics.getOrCreateCounter(MetricsConstants.TOTAL_NUM_ABORTED_TXNS).inc(txnids.size());
      }
+      LOG.info("Txns are aborted successfully due to : {}", errorMsg);


The error message is not informative: something aborted because of ...
Maybe print ids of aborted txns? Make sure we are not doing unnecessary logging. see

performTimeOuts() LOG.info("Aborted the following transactions due to timeout: {}", batchToAbort.toString()); LOG.info("Aborted {} transactions due to timeout", numTxnsAborted);

and

commitTxn() String msg = "Aborting [" + JavaUtils.txnIdToString(txnid) + "," + commitId + "]" + " due to a write conflict on " + resource + " committed by " + committedTxn + " " + rs.getString(7) + "/" + rs.getString(8);

For performTimeOuts()
Removed this log - LOG.info("Aborted the following transactions due to timeout: {}", batchToAbort.toString());

For commitTxn()
Changed this log to include only the committed transaction details and not the ones that are going to be aborted.

Added aborted transaction IDs in the generic log.

deniskuzZ · 2023-01-05T10:43:01Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

      if (MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
        Metrics.getOrCreateCounter(MetricsConstants.TOTAL_NUM_ABORTED_TXNS).inc(txnids.size());
      }
+      LOG.info("Txns are aborted successfully due to : {}", errorMsg);


Could we populate TXN_META_INFO with aborted reason as well?
see //todo: should make abortTxns() write something into TXNS.TXN_META_INFO about this

You had mentioned that we should avoid persisting due to the below reason -
#3656 (comment)

Simhadri wanted to add a new column in TXNS table for that, but looks like we already have one TXN_META_INFO. In some situations that might be useful, but I'll leave it up to you.

I have added the message contents in this column. Done.

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/Msck.java

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java

deniskuzZ · 2023-01-09T10:22:24Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

+          errorMsg = TxnErrorMsg.ABORT_REPLAYED_REPL_TXN;
+        } else if (isHiveReplTxn) {
+          errorMsg = TxnErrorMsg.ABORT_DEFAULT_REPL_TXN;
+        } else if (rqst.isSetErrorCode() && rqst.getErrorCode() == TxnErrorMsg.ABORT_ROLLBACK.getErrorCode()) {


what is this? why not

if (rqst.isSetErrorCode() { errorMsg = TxnErrorMsg.getErrorMsg(rqst.getErrorCode()) }

should isReplayedReplTxn override rqst.getErrorCode()?

Updated the condition to -
if (rqst.isSetErrorCode() { errorMsg = TxnErrorMsg.getErrorMsg(rqst.getErrorCode()) }

As far as isReplayedReplTxn is concerned. There was no such intention of keeping it first in if else block.
Please note that isReplayedReplTxn & isHiveReplTxn is set only when request is passed through replRollbackTxn() call in the MetastoreClient.

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

deniskuzZ · 2023-01-09T10:31:37Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

            numTxnsAborted += batchToAbort.size();
            //todo: add TXNS.COMMENT filed and set it to 'aborted by system due to timeout'
            Collections.sort(batchToAbort);//easier to read logs
-            LOG.info("Aborted the following transactions due to timeout: {}", batchToAbort.toString());


above sorting was only needed for the logging, once you removed it, it became redundant, maybe it should be moved to abort method

line 5790 prints the same message

Added sort() in the generic logs section.
Line 5790 - This log prints the number of aborted txns. Included this info (number of aborted txns) in the generic logs as well.

streaming/src/java/org/apache/hive/streaming/TransactionBatch.java

deniskuzZ · 2023-01-09T10:54:56Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

      if (MetastoreConf.getBoolVar(conf, MetastoreConf.ConfVars.METASTORE_ACIDMETRICS_EXT_ON)) {
        Metrics.getOrCreateCounter(MetricsConstants.TOTAL_NUM_ABORTED_TXNS).inc(txnids.size());
      }
+      LOG.warn("Txns are aborted successfully due to : {}. Aborted Transaction IDs : {}", errorMsg, txnids);


LOG.info("Aborted transactions {} due to {}", txnids, errorMsg);

errorMsg should be just text, like "heartbeat time-out"
either update the msg in TxnErrorMsg or use it as a pattern for the string formatter

ABORT_ROLLBACK(50005, "<<<Txn aborted due to>>> rollback"),

Used this format of logging as suggested -
LOG.info("Aborted transactions {} due to {}", txnids, errorMsg);

Updated the error msgs to make it concise in TxnErrorMsg.

deniskuzZ · 2023-01-09T10:56:19Z

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

    if (txnids.isEmpty()) {
      return 0;
    }
+    LOG.debug("Trying to abort txns due to : {}. Aborted Transaction IDs : {}", errorMsg, txnids);


LOG.debug("Trying to abort txns {} due to {}", txnids, errorMsg)
see above comment

Implemented as suggested.

deniskuzZ

LGTM, pending tests
Please check that "Aborting {} transaction(s) {} due to {}", txnids.size(), txnids, errorMsg
prints the list properly.

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java

SourabhBadhya · 2023-01-10T04:51:20Z

LGTM, pending tests Please check that "Aborting {} transaction(s) {} due to {}", txnids.size(), txnids, errorMsg prints the list properly.

Just checked this. It displays in this way -
Aborted 2 transaction(s) [1234, 5678] due to abort by query command

sonarqubecloud · 2023-01-10T06:26:03Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

No Coverage information
No Duplication information

…dhya, reviewed by Denys Kuzmenko, Laszlo Vegh) Closes apache#3908

github-actions bot requested review from klcopp and miklosgergely January 4, 2023 06:38

kgyrtkirk added tests pending tests unstable and removed tests pending tests unstable labels Jan 4, 2023

veghlaci05 requested changes Jan 4, 2023

View reviewed changes

veghlaci05 approved these changes Jan 4, 2023

View reviewed changes

kgyrtkirk added tests failed tests pending tests passed and removed tests pending tests failed labels Jan 4, 2023

deniskuzZ reviewed Jan 5, 2023

View reviewed changes

Sourabh Badhya added 3 commits January 9, 2023 13:57

HIVE-11495: Add aborted reason to transaction information

eec8d60

Address review comments - 1

76404e9

Address review comments - 2

c76271a

SourabhBadhya force-pushed the HIVE-11495 branch from 3c2be0b to c76271a Compare January 9, 2023 08:28

kgyrtkirk added tests pending and removed tests passed labels Jan 9, 2023

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/Msck.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/Msck.java Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

streaming/src/java/org/apache/hive/streaming/TransactionBatch.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

kgyrtkirk added tests unstable and removed tests pending labels Jan 9, 2023

Address review comments - 3

d7c35bc

kgyrtkirk added tests pending and removed tests unstable labels Jan 9, 2023

deniskuzZ approved these changes Jan 9, 2023

View reviewed changes

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

...etastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnHandler.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java Outdated Show resolved Hide resolved

deniskuzZ reviewed Jan 9, 2023

View reviewed changes

...tastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/txn/TxnErrorMsg.java Outdated Show resolved Hide resolved

kgyrtkirk added tests unstable and removed tests pending labels Jan 9, 2023

Address review comments - 4

d4223fa

kgyrtkirk added tests pending and removed tests unstable labels Jan 10, 2023

kgyrtkirk added tests passed and removed tests pending labels Jan 10, 2023

deniskuzZ merged commit 3cba64f into apache:master Jan 10, 2023

yeahyung pushed a commit to yeahyung/hive that referenced this pull request Jul 20, 2023

HIVE-11495: Add aborted reason to transaction information (Sourabh Ba…

fdff1db

…dhya, reviewed by Denys Kuzmenko, Laszlo Vegh) Closes apache#3908

tarak271 pushed a commit to tarak271/hive-1 that referenced this pull request Dec 19, 2023

HIVE-11495: Add aborted reason to transaction information (Sourabh Ba…

66c8e33

…dhya, reviewed by Denys Kuzmenko, Laszlo Vegh) Closes apache#3908

HIVE-11495: Add aborted reason to transaction information #3908

HIVE-11495: Add aborted reason to transaction information #3908

Uh oh!

Conversation

SourabhBadhya commented Jan 4, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

veghlaci05 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SourabhBadhya Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SourabhBadhya commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veghlaci05 commented Jan 4, 2023

Uh oh!

veghlaci05 left a comment

Choose a reason for hiding this comment

Uh oh!

deniskuzZ Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniskuzZ Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniskuzZ Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deniskuzZ Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

SourabhBadhya Jan 4, 2023 •

edited

Loading

SourabhBadhya commented Jan 4, 2023 •

edited

Loading

deniskuzZ Jan 5, 2023 •

edited

Loading

deniskuzZ Jan 5, 2023 •

edited

Loading

deniskuzZ Jan 5, 2023 •

edited

Loading

deniskuzZ Jan 9, 2023 •

edited

Loading

deniskuzZ Jan 9, 2023 •

edited

Loading

deniskuzZ Jan 9, 2023 •

edited

Loading

deniskuzZ Jan 9, 2023 •

edited

Loading

SourabhBadhya commented Jan 10, 2023 •

edited

Loading