[GitHub] [shardingsphere] cherrylzhao commented on a change in pull request #6042: optimize lifecycle management of ShardingTransactionType

[GitBox <gi...@apache.org>]

cherrylzhao commented on a change in pull request #6042:
URL: https://github.com/apache/shardingsphere/pull/6042#discussion_r440539613



##########
File path: examples/shardingsphere-jdbc-example/transaction-example/transaction-2pc-xa-bitronix-raw-jdbc-example/src/main/java/org/apache/shardingsphere/example/transaction/xa/bitronix/raw/jdbc/OrderServiceImpl.java
##########
@@ -65,6 +65,8 @@ public void processSuccess() throws SQLException {
             doInsert(preparedStatement);
             connection.commit();
             System.out.println("INSERT 10 orders success");
+        } finally {
+            TransactionTypeHolder.clear();

Review comment:
       ThreadLocal value will be cleared in connection.close method, see more detail in `AbstractConndectionAdapter#close`

##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-core/src/main/java/org/apache/shardingsphere/driver/jdbc/adapter/AbstractConnectionAdapter.java
##########
@@ -74,7 +73,7 @@
     private volatile boolean closed;
     
     private int transactionIsolation = TRANSACTION_READ_UNCOMMITTED;
-    
+

Review comment:
       should keep incident in new line

##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-spring/shardingsphere-jdbc-transaction-spring/src/main/java/org/apache/shardingsphere/spring/transaction/ShardingTransactionTypeInterceptor.java
##########
@@ -37,8 +38,16 @@
     public Object invoke(final MethodInvocation methodInvocation) throws Throwable {
         ShardingTransactionType shardingTransactionType = getAnnotation(methodInvocation);
         Objects.requireNonNull(shardingTransactionType, "could not found sharding transaction type annotation");
+        TransactionType preTransactionType = TransactionTypeHolder.get();
         TransactionTypeHolder.set(shardingTransactionType.value());
-        return methodInvocation.proceed();
+        try {
+            return methodInvocation.proceed();
+        } finally {
+            TransactionTypeHolder.clear();
+            if (preTransactionType != null) {

Review comment:
       for checkstyle: `preTransactionType != null` => `null != preTransactionType`

##########
File path: shardingsphere-jdbc/shardingsphere-jdbc-spring/shardingsphere-jdbc-transaction-spring/src/main/java/org/apache/shardingsphere/spring/transaction/ShardingTransactionTypeInterceptor.java
##########
@@ -37,8 +38,16 @@
     public Object invoke(final MethodInvocation methodInvocation) throws Throwable {
         ShardingTransactionType shardingTransactionType = getAnnotation(methodInvocation);
         Objects.requireNonNull(shardingTransactionType, "could not found sharding transaction type annotation");
+        TransactionType preTransactionType = TransactionTypeHolder.get();

Review comment:
       it's better to add a unit test to verify this feature. current implementation will not work in multiple @ShardingTransactionType ?




----------------------------------------------------------------
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:
users@infra.apache.org