This is an automated email from the ASF dual-hosted git repository. morningman pushed a commit to branch dev-1.0.0 in repository https://gitbox.apache.org/repos/asf/incubator-doris.git
commit 8a2da556e755e6ec4f5afb83b8807c6cbb7874b1 Author: caiconghui <[email protected]> AuthorDate: Fri Mar 11 17:36:23 2022 +0800 [fix](transaction) Fix committed transaction couldn't be finished when table is dropped (#8423) Issue Number: close #8426 --- .../java/org/apache/doris/catalog/Database.java | 19 ++++++----- .../main/java/org/apache/doris/catalog/Table.java | 9 ++--- .../apache/doris/common/util/MetaLockUtils.java | 12 +++++++ .../apache/doris/service/FrontendServiceImpl.java | 2 +- .../doris/transaction/DatabaseTransactionMgr.java | 38 +++++++++++++--------- .../doris/transaction/PublishVersionDaemon.java | 7 ++-- 6 files changed, 53 insertions(+), 34 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java index db93dd9..c1d55c8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Database.java @@ -411,14 +411,16 @@ public class Database extends MetaObject implements Writable { return views; } - public List<Table> getTablesOnIdOrderOrThrowException(List<Long> tableIdList) throws MetaNotFoundException { - List<Table> tableList = Lists.newArrayList(); + /** + * this method is used for get existed table list by table id list, if table not exist, just ignore it. + */ + public List<Table> getTablesOnIdOrderIfExist(List<Long> tableIdList) { + List<Table> tableList = Lists.newArrayListWithCapacity(tableIdList.size()); for (Long tableId : tableIdList) { Table table = idToTable.get(tableId); - if (table == null) { - throw new MetaNotFoundException("unknown table, tableId=" + tableId); + if (table != null) { + tableList.add(table); } - tableList.add(table); } if (tableList.size() > 1) { return tableList.stream().sorted(Comparator.comparing(Table::getId)).collect(Collectors.toList()); @@ -426,13 +428,12 @@ public class Database extends MetaObject implements Writable { return tableList; } - public List<Table> getTablesOnIdOrderWithIgnoringWrongTableId(List<Long> tableIdList) { - List<Table> tableList = Lists.newArrayList(); + public List<Table> getTablesOnIdOrderOrThrowException(List<Long> tableIdList) throws MetaNotFoundException { + List<Table> tableList = Lists.newArrayListWithCapacity(tableIdList.size()); for (Long tableId : tableIdList) { Table table = idToTable.get(tableId); if (table == null) { - LOG.warn("unknown table, tableId=" + tableId); - continue; + throw new MetaNotFoundException("unknown table, tableId=" + tableId); } tableList.add(table); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Table.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Table.java index a0c1fb1..58cf08a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Table.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Table.java @@ -170,11 +170,12 @@ public class Table extends MetaObject implements Writable { } public boolean writeLockIfExist() { - if (!isDropped) { - this.rwLock.writeLock().lock(); - return true; + this.rwLock.writeLock().lock(); + if (isDropped) { + this.rwLock.writeLock().unlock(); + return false; } - return false; + return true; } public boolean tryWriteLock(long timeout, TimeUnit unit) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java b/fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java index 42346a9..42fa8cd 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java @@ -21,6 +21,8 @@ import org.apache.doris.catalog.Database; import org.apache.doris.catalog.Table; import org.apache.doris.common.MetaNotFoundException; +import com.google.common.collect.Lists; + import java.util.List; import java.util.concurrent.TimeUnit; @@ -61,6 +63,16 @@ public class MetaLockUtils { } } + public static List<Table> writeLockTablesIfExist(List<Table> tableList) { + List<Table> lockedTablesList = Lists.newArrayListWithCapacity(tableList.size()); + for (Table table : tableList) { + if (table.writeLockIfExist()) { + lockedTablesList.add(table); + } + } + return lockedTablesList; + } + public static void writeLockTablesOrMetaException(List<Table> tableList) throws MetaNotFoundException { for (int i = 0; i < tableList.size(); i++) { try { diff --git a/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java b/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java index 6628e97..56d0cfd 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java +++ b/fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java @@ -918,7 +918,7 @@ public class FrontendServiceImpl implements FrontendService.Iface { throw new UserException("transaction [" + request.getTxnId() + "] not found"); } List<Long> tableIdList = transactionState.getTableIdList(); - List<Table> tableList = database.getTablesOnIdOrderWithIgnoringWrongTableId(tableIdList); + List<Table> tableList = database.getTablesOnIdOrderOrThrowException(tableIdList); for (Table table : tableList) { // check auth checkPasswordAndPrivs(cluster, request.getUser(), request.getPasswd(), request.getDb(), diff --git a/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java b/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java index 6b106e1..f6b6d2c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/transaction/DatabaseTransactionMgr.java @@ -769,22 +769,17 @@ public class DatabaseTransactionMgr { errorReplicaIds.addAll(originalErrorReplicas); } - Database db = catalog.getDbNullable(transactionState.getDbId()); - if (db == null) { - writeLock(); - try { - transactionState.setTransactionStatus(TransactionStatus.ABORTED); - transactionState.setReason("db is dropped"); - LOG.warn("db is dropped during transaction, abort transaction {}", transactionState); - unprotectUpsertTransactionState(transactionState, false); - return; - } finally { - writeUnlock(); - } - } + // case 1 If database is dropped, then we just throw MetaNotFoundException, because all related tables are already force dropped, + // we just ignore the transaction with all tables been force dropped. + // case 2 If at least one table lock successfully, which means that the transaction should be finished for the existed tables + // while just ignore tables which have been dropped forcefully. + // case 3 Database exist and all tables already been dropped, this case is same with case1, just finish the transaction with empty commit info + // only three cases mentioned above may happen, because user cannot drop table without force while there are committed transactions on table + // and writeLockTablesIfExist is a blocking function, the returned result would be the existed table list which hold write lock + Database db = catalog.getDbOrMetaException(transactionState.getDbId()); List<Long> tableIdList = transactionState.getTableIdList(); - List<Table> tableList = db.getTablesOnIdOrderOrThrowException(tableIdList); - MetaLockUtils.writeLockTablesOrMetaException(tableList); + List<Table> tableList = db.getTablesOnIdOrderIfExist(tableIdList); + tableList = MetaLockUtils.writeLockTablesIfExist(tableList); try { boolean hasError = false; Iterator<TableCommitInfo> tableCommitInfoIterator = transactionState.getIdToTableCommitInfos().values().iterator(); @@ -1651,11 +1646,19 @@ public class DatabaseTransactionMgr { } public void replayUpsertTransactionState(TransactionState transactionState) throws MetaNotFoundException { + boolean shouldAddTableListLock = transactionState.getTransactionStatus() == TransactionStatus.COMMITTED || + transactionState.getTransactionStatus() == TransactionStatus.VISIBLE; + Database db = null; + List<Table> tableList = null; + if (shouldAddTableListLock) { + db = catalog.getDbOrMetaException(transactionState.getDbId()); + tableList = db.getTablesOnIdOrderIfExist(transactionState.getTableIdList()); + tableList = MetaLockUtils.writeLockTablesIfExist(tableList); + } writeLock(); try { // set transaction status will call txn state change listener transactionState.replaySetTransactionStatus(); - Database db = catalog.getDbOrMetaException(transactionState.getDbId()); if (transactionState.getTransactionStatus() == TransactionStatus.COMMITTED) { LOG.info("replay a committed transaction {}", transactionState); updateCatalogAfterCommitted(transactionState, db); @@ -1666,6 +1669,9 @@ public class DatabaseTransactionMgr { unprotectUpsertTransactionState(transactionState, true); } finally { writeUnlock(); + if (shouldAddTableListLock) { + MetaLockUtils.writeUnlockTables(tableList); + } } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/transaction/PublishVersionDaemon.java b/fe/fe-core/src/main/java/org/apache/doris/transaction/PublishVersionDaemon.java index a382fac..6011919 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/transaction/PublishVersionDaemon.java +++ b/fe/fe-core/src/main/java/org/apache/doris/transaction/PublishVersionDaemon.java @@ -27,7 +27,6 @@ import org.apache.doris.catalog.Table; import org.apache.doris.catalog.Tablet; import org.apache.doris.catalog.TabletInvertedIndex; import org.apache.doris.common.Config; -import org.apache.doris.common.UserException; import org.apache.doris.common.util.MasterDaemon; import org.apache.doris.task.AgentBatchTask; import org.apache.doris.task.AgentTaskExecutor; @@ -73,10 +72,10 @@ public class PublishVersionDaemon extends MasterDaemon { return true; } - private void publishVersion() throws UserException { + private void publishVersion() { GlobalTransactionMgr globalTransactionMgr = Catalog.getCurrentGlobalTransactionMgr(); List<TransactionState> readyTransactionStates = globalTransactionMgr.getReadyToPublishTransactions(); - if (readyTransactionStates == null || readyTransactionStates.isEmpty()) { + if (readyTransactionStates.isEmpty()) { return; } @@ -242,7 +241,7 @@ public class PublishVersionDaemon extends MasterDaemon { // one transaction exception should not affect other transaction globalTransactionMgr.finishTransaction(transactionState.getDbId(), transactionState.getTransactionId(), publishErrorReplicaIds); } catch (Exception e) { - LOG.warn("error happends when finish transaction {} ", transactionState.getTransactionId(), e); + LOG.warn("error happens when finish transaction {}", transactionState.getTransactionId(), e); } if (transactionState.getTransactionStatus() != TransactionStatus.VISIBLE) { // if finish transaction state failed, then update publish version time, should check --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
