HIVE-28916: Fix unbalannced calls in ObjectStore rollback transaction #5780
+124
−75
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -561,17 +561,10 @@ public void shutdown() { | |
| @Override | ||
| public boolean openTransaction() { | ||
| openTrasactionCalls++; | ||
| if (currentTransaction == null || !currentTransaction.isActive()){ | ||
|
There was a problem hiding this comment. Why do we check for There was a problem hiding this comment. To open transaction if it's not opened. It would reopen a new transaction if has already called rollback. There was a problem hiding this comment. if the interior txn was rolled back, should we rollback everything? |
||
| currentTransaction = pm.currentTransaction(); | ||
| currentTransaction.begin(); | ||
| transactionStatus = TXN_STATUS.OPEN; | ||
| } | ||
|
|
||
| boolean result = currentTransaction.isActive(); | ||
|
|
@@ -629,13 +622,13 @@ public boolean commitTransaction() { | |
| LOG.error("Unbalanced calls to open/commit Transaction", e); | ||
| throw e; | ||
| } | ||
|
|
||
| debugLog("Commit transaction: count = " + openTrasactionCalls + ", isactive "+ currentTransaction.isActive()); | ||
| if ((openTrasactionCalls == 1) && currentTransaction.isActive()) { | ||
| transactionStatus = TXN_STATUS.COMMITED; | ||
| currentTransaction.commit(); | ||
| } | ||
| openTrasactionCalls--; | ||
|
There was a problem hiding this comment. what if commit throws RuntimeException, db txn would be closed, but openTrasactionCalls won't be 0 There was a problem hiding this comment. If commit throw RuntimeException, it should call rollback in the finally block of previous layer where the openTransactionCalls would be decreased. |
||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -662,7 +655,7 @@ public void rollbackTransaction() { | |
| currentTransaction.rollback(); | ||
| } | ||
| } finally { | ||
| openTrasactionCalls--; | ||
|
There was a problem hiding this comment. after rollback openTrasactionCalls should be 0 There was a problem hiding this comment. 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() # place2There was a problem hiding this comment. that is what we support now, isn't ? the inner transactions are somewhat similar to @transactional(REQUIRED) annotation |
||
| transactionStatus = TXN_STATUS.ROLLBACK; | ||
| // remove all detached objects from the cache, since the transaction is | ||
| // being rolled back they are no longer relevant, and this prevents them | ||
|
|
@@ -3254,31 +3247,38 @@ protected List<Partition> getJdoResult(GetHelper<List<Partition>> ctx) throws Me | |
| public Partition getPartitionWithAuth(String catName, String dbName, String tblName, | ||
| List<String> partVals, String user_name, List<String> group_names) | ||
| throws NoSuchObjectException, MetaException, InvalidObjectException { | ||
| return new GetHelper<Partition>(catName, dbName, tblName, false, true) { | ||
|
There was a problem hiding this comment. It may throw exception, so we need such logic: try {
openTransaction();
success = commitTransaction();
} finally {
if (!success) rollbackTransaction();
}There was a problem hiding this comment. does it even run in txn? where do we open it? |
||
| @Override | ||
| protected String describeResult() { | ||
| return "partition with auth"; | ||
| } | ||
|
|
||
| @Override | ||
| protected Partition getSqlResult(GetHelper<Partition> ctx) throws MetaException { | ||
| throw new UnsupportedOperationException("UnsupportedOperationException"); | ||
| } | ||
|
|
||
| @Override | ||
| protected Partition getJdoResult(GetHelper<Partition> ctx) | ||
| throws MetaException, NoSuchObjectException, InvalidObjectException, InvalidInputException { | ||
| MPartition mpart = getMPartition(catName, dbName, tblName, partVals, null); | ||
| if (mpart == null) { | ||
| throw new NoSuchObjectException("partition values=" | ||
| + partVals.toString()); | ||
| } | ||
| MTable mtbl = mpart.getTable(); | ||
|
|
||
| Partition part = convertToPart(catName, dbName, tblName, mpart, TxnUtils.isAcidTable(mtbl.getParameters())); | ||
| if ("TRUE".equalsIgnoreCase(mtbl.getParameters().get("PARTITION_LEVEL_PRIVILEGE"))) { | ||
| String partName = Warehouse.makePartName(convertToFieldSchemas(mtbl | ||
| .getPartitionKeys()), partVals); | ||
| PrincipalPrivilegeSet partAuth = getPartitionPrivilegeSet(catName, dbName, | ||
| tblName, partName, user_name, group_names); | ||
| part.setPrivileges(partAuth); | ||
| } | ||
| return part; | ||
| } | ||
| }.run(false); | ||
| } | ||
|
|
||
| private List<Partition> convertToParts(String catName, String dbName, String tblName, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to open txn before the
PreAlterCatalogEvent? are those transactional as well?