HIVE-28916: Fix unbalannced calls in ObjectStore rollback transaction #5780

wecharyu · 2025-04-21T17:01:47Z

What changes were proposed in this pull request?

Fix the bug in ObjectStore#openTransaction and ObjectStore#rollbackTransaction.

Why are the changes needed?

Currently rollbackTransaction() always reset openTransactionCalls to 0, but it would cause unbalanced issue in nested transaction calls, for example:

openTransaction()
  // new function1
  {
    openTransaction()
    commitTransaction() # place1
  }

  // new function2
  {
    openTransaction()
    commitTransaction()
  }
commitTransaction() # place2

If the new function1 rollbackTransaction in place1, then it would get unbalanced exception in place2.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add a unit test:

mvn test -Dtest.groups= -Dtest=org.apache.hadoop.hive.metastore.TestObjectStore#testNestedTransaction -pl :hive-standalone-metastore-server

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

dengzhhu653 · 2025-04-24T03:33:12Z

Some thoughts:

at place1, the openTrasactionCalls should be 2, so commitTransaction is just changing the openTrasactionCalls to 1, we don't reach the end of the transaction here.
if we rollback at the place1, doesn't it mean we get an unexpected exception here, why do we continue to execute to place2?

wecharyu · 2025-04-29T15:46:56Z

if we rollback at the place1, doesn't it mean we get an unexpected exception here, why do we continue to execute to place2?

@dengzhhu653 There are some cases where rollback transaction does not need fail the api calls, for example create_xxx will always check if it already exists by get_xxx that throw expected NoSuchObjectException, in this case we should continue other queries.

sonarqubecloud · 2025-04-29T16:42:23Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dengzhhu653 · 2025-04-30T01:07:23Z

if we rollback at the place1, doesn't it mean we get an unexpected exception here, why do we continue to execute to place2?

@dengzhhu653 There are some cases where rollback transaction does not need fail the api calls, for example create_xxx will always check if it already exists by get_xxx that throw expected NoSuchObjectException, in this case we should continue other queries.

Per my understanding, this shouldn't happen, imaging we rollback the transaction at place1, then the ObjectStore openTrasactionCalls is reset to 0, if we continue to execute, then means we have one openTransaction and two commitTransaction, the commitTransaction at place2 should throw the exception:

if (openTrasactionCalls <= 0) {
      RuntimeException e = new RuntimeException("commitTransaction was called but openTransactionCalls = "
          + openTrasactionCalls + ". This probably indicates that there are unbalanced " +
          "calls to openTransaction/commitTransaction");
      LOG.error("Unbalanced calls to open/commit Transaction", e);
      throw e;
 }

the Metastore client call should get this exception, but our tests work fine.

wecharyu · 2025-04-30T05:03:58Z

Per my understanding, this shouldn't happen, imaging we rollback the transaction at place1, then the ObjectStore
openTrasactionCalls is reset to 0, if we continue to execute, then means we have one openTransaction and two commitTransaction, the commitTransaction at place2 should throw the exception:
if (openTrasactionCalls <= 0) {
     RuntimeException e = new RuntimeException("commitTransaction was called but openTransactionCalls = "
         + openTrasactionCalls + ". This probably indicates that there are unbalanced " +
         "calls to openTransaction/commitTransaction");
     LOG.error("Unbalanced calls to open/commit Transaction", e);
     throw e;
}
the Metastore client call should get this exception, but our tests work fine.

Yes it works now because current code does not put get_xxx and create_xxx into the same transaction, it would fail if we openTransaction at the top of a create_xxx api.

Additionally, the failed tests in this PR reveal following unbalanced calls that are hidden by current implementation:

commitTransaction() twice for a openTransaction()
rollbackTransaction() even it does not call openTransaction()

deniskuzZ · 2025-05-08T12:51:58Z

...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java

@@ -1299,7 +1297,9 @@ private void create_database_core(RawStore ms, final Database db)
      success = ms.commitTransaction();
    } finally {
      if (!success) {
-        ms.rollbackTransaction();
+        if (openTransaction) {


why are you checking for an openTransaction flag here? I don't see the same check in other places.

if (isActiveTransaction() && transactionStatus != TXN_STATUS.ROLLBACK) { currentTransaction.rollback(); }

In create_database_core it may throw exception before calling openTransaction(), so it may not need call rollbackTransaction() in finally block.

deniskuzZ · 2025-05-08T12:52:45Z

...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java

      ms.openTransaction();
+      firePreEvent(new PreCreateDataConnectorEvent(connector, this));


why the change?

ditto, firePreEvent may throw exception and not call openTransaction().

deniskuzZ · 2025-05-08T12:54:03Z

...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java

@@ -983,12 +983,11 @@ public void alter_catalog(AlterCatalogRequest rqst) throws TException {
    GetCatalogResponse oldCat = null;

    try {
+      ms.openTransaction();


why do we need to open txn before the PreAlterCatalogEvent? are those transactional as well?

deniskuzZ · 2025-05-08T12:57:08Z

...astore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveAlterHandler.java

@@ -191,6 +184,10 @@ public void alterTable(RawStore msdb, Warehouse wh, String catName, String dbnam
            TableName.getQualified(catName, dbname, name) + " doesn't exist");
      }

+      String expectedKey = environmentContext != null && environmentContext.getProperties() != null ?


why move non-transactional logic under the txn boundary?

deniskuzZ · 2025-05-08T13:00:05Z

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

@@ -561,17 +561,10 @@ public void shutdown() {
  @Override
  public boolean openTransaction() {
    openTrasactionCalls++;
-    if (openTrasactionCalls == 1) {
+    if (currentTransaction == null || !currentTransaction.isActive()){


Why do we check for !currentTransaction.isActive()? What if we called rollback on the interior txn?

To open transaction if it's not opened. It would reopen a new transaction if has already called rollback.

deniskuzZ · 2025-05-08T13:07:34Z

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

      transactionStatus = TXN_STATUS.COMMITED;
      currentTransaction.commit();
    }
+    openTrasactionCalls--;


what if commit throws RuntimeException, db txn would be closed, but openTrasactionCalls won't be 0

If commit throw RuntimeException, it should call rollback in the finally block of previous layer where the openTransactionCalls would be decreased.

deniskuzZ · 2025-05-08T13:08:34Z

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

@@ -662,7 +655,7 @@ public void rollbackTransaction() {
        currentTransaction.rollback();
      }
    } finally {
-      openTrasactionCalls = 0;
+      openTrasactionCalls--;


after rollback openTrasactionCalls should be 0

Our unit test could not pass if is 0 here, the key point seems that if we should support such usage:

openTransaction() // new function1 { openTransaction() commitTransaction() # place1 } // new function2 { openTransaction() commitTransaction() } commitTransaction() # place2

deniskuzZ · 2025-05-08T13:10:29Z

...e-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java

-        commitTransaction();
-        throw new NoSuchObjectException("partition values="
-            + partVals.toString());
+    return new GetHelper<Partition>(catName, dbName, tblName, false, true) {


why refactor?

It may throw exception, so we need such logic:

try { openTransaction(); success = commitTransaction(); } finally { if (!success) rollbackTransaction(); }

HIVE-28916: Fix unbalannced calls in ObjectStore rollback transaction

62c819f

asf-ci-hive added tests pending tests unstable and removed tests pending labels Apr 21, 2025

fix tests

c057147

asf-ci-hive added tests pending and removed tests unstable labels Apr 22, 2025

deniskuzZ reviewed Apr 22, 2025

View reviewed changes

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

asf-ci-hive added tests unstable and removed tests pending labels Apr 22, 2025

fix unbalanced calls

bc595b7

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Apr 23, 2025

fix tests

d57cb28

asf-ci-hive added tests pending and removed tests unstable labels Apr 29, 2025

asf-ci-hive added tests failed and removed tests pending labels Apr 29, 2025

deniskuzZ reviewed May 8, 2025

View reviewed changes

		ms.openTransaction();
		firePreEvent(new PreCreateDataConnectorEvent(connector, this));

HIVE-28916: Fix unbalannced calls in ObjectStore rollback transaction #5780

Are you sure you want to change the base?

HIVE-28916: Fix unbalannced calls in ObjectStore rollback transaction #5780

Conversation

wecharyu commented Apr 21, 2025

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!

Uh oh!

dengzhhu653 commented Apr 24, 2025

Uh oh!

wecharyu commented Apr 29, 2025

Uh oh!

sonarqubecloud bot commented Apr 29, 2025

Quality Gate passed

Uh oh!

dengzhhu653 commented Apr 30, 2025

Uh oh!

wecharyu commented Apr 30, 2025

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 May 8, 2025 • 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!

deniskuzZ May 8, 2025 • 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!

Uh oh!

deniskuzZ May 8, 2025 •

edited

Loading

deniskuzZ May 8, 2025 •

edited

Loading