morningman commented on a change in pull request #3369:
URL: https://github.com/apache/incubator-doris/pull/3369#discussion_r419821836
##########
File path: fe/src/main/java/org/apache/doris/service/FrontendServiceImpl.java
##########
@@ -800,8 +800,8 @@ private void loadTxnRollbackImpl(TLoadTxnRollbackRequest
request) throws UserExc
checkPasswordAndPrivs(cluster, request.getUser(),
request.getPasswd(), request.getDb(),
request.getTbl(), request.getUser_ip(),
PrivPredicate.LOAD);
}
-
-
Catalog.getCurrentGlobalTransactionMgr().abortTransaction(request.getTxnId(),
+ long dbId = Catalog.getInstance().getDb(request.getDb()).getId();
Review comment:
`getDb()` may return null if database does not exist yet.
##########
File path:
fe/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.java
##########
@@ -123,6 +95,22 @@ public TxnStateCallbackFactory getCallbackFactory() {
return callbackFactory;
}
+ public DatabaseTransactionMgr getDatabaseTransactioMgr(long dbId) {
+ DatabaseTransactionMgr dbTransactionMgr =
dbIdToDatabaseTransactionMgrs.get(dbId);
+ if (dbTransactionMgr == null) {
+ throw new NullPointerException("databaseTransactionMgr[" + dbId +
"] does not exist");
+ }
+ return dbTransactionMgr;
+ }
+
+ public void addDatabaseTransactionMgr(Long dbId, EditLog editLog) {
+ dbIdToDatabaseTransactionMgrs.put(dbId, new
DatabaseTransactionMgr(dbId, editLog));
Review comment:
Is it more safe to use `putIfAbsent()` method?
##########
File path:
fe/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.java
##########
@@ -123,6 +95,22 @@ public TxnStateCallbackFactory getCallbackFactory() {
return callbackFactory;
}
+ public DatabaseTransactionMgr getDatabaseTransactioMgr(long dbId) {
+ DatabaseTransactionMgr dbTransactionMgr =
dbIdToDatabaseTransactionMgrs.get(dbId);
+ if (dbTransactionMgr == null) {
+ throw new NullPointerException("databaseTransactionMgr[" + dbId +
"] does not exist");
Review comment:
Not using `NullPointerException`, you can use `TransactionException`
##########
File path:
fe/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.java
##########
@@ -194,14 +184,14 @@ public long beginTransaction(long dbId, List<Long>
tableIdList, String label, TU
}
}
- checkRunningTxnExceedLimit(dbId, sourceType);
+ checkRunningTxnExceedLimit(dbTransactionMgr, sourceType);
Review comment:
I think `checkRunningTxnExceedLimit()` this method can be moved into the
`DbTransactionMgr`
##########
File path:
fe/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.java
##########
@@ -500,8 +485,15 @@ public boolean commitAndPublishTransaction(Database db,
long transactionId,
} finally {
db.writeUnlock();
}
-
- TransactionState transactionState =
idToTransactionState.get(transactionId);
+ DatabaseTransactionMgr dbTransactionMgr =
getDatabaseTransactioMgr(db.getId());
+ TransactionState transactionState = null;
+ dbTransactionMgr.readLock();
Review comment:
In fact, you will find that all the logic in `GlobalTransactionMgr` is
now executed in `DbTransactionMgr`. `GlobalTransactionMgr` has only become a
class for managing `DbTransactionMgr`.
The usage patterns of most methods are:
```
DbTransactionMgr dbMgr = getDbTransactionMgr(dbId);
dbMgr.lock()
try {
dbMgr.doSomeTxnThing();
} finally {
dbMgr.unlock();
}
```
Therefore, I think it is more appropriate to move all the main
implementation logic into `DbTransactionMgr`. In this way, the responsibilities
of `GlobalTransactionMgr` and `DbTransactionMgr` will be clearer.
`GlobalTransactionMgr` only serves as the entry class for transaction
operations and is responsible for managing `DbTransactionMgr`. And
`DbTransactionMgr` is the actual transaction operation class.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
