[Feature] add transaction hook #525

github-ygy · 2019-03-05T13:24:23Z

Ⅰ. Describe what this PR did

Feature add transaction hook

Ⅱ. Does this pull request fix one issue?

add transaction hook in the Transaction Life Cycle

Ⅲ. Why don't you add test cases (unit test/integration test)?

add test in TransactionHookManagerTest
add test in TransactionTemplateTest

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

…ure_add_hook_0304

codecov-io · 2019-03-05T13:28:32Z

Codecov Report

Merging #525 into develop will increase coverage by 0.38%.
The diff coverage is 68.75%.

@@              Coverage Diff              @@
##             develop     #525      +/-   ##
=============================================
+ Coverage      31.89%   32.28%   +0.38%     
- Complexity       775      798      +23     
=============================================
  Files            200      202       +2     
  Lines           7948     8032      +84     
  Branches         939      941       +2     
=============================================
+ Hits            2535     2593      +58     
- Misses          5155     5178      +23     
- Partials         258      261       +3

Impacted Files	Coverage Δ	Complexity Δ
...car/tm/api/transaction/TransactionHookAdapter.java	`12.5% <12.5%> (ø)`	`1 <1> (?)`
...car/tm/api/transaction/TransactionHookManager.java	`66.66% <66.66%> (ø)`	`5 <5> (?)`
...m/alibaba/fescar/tm/api/TransactionalTemplate.java	`75% <75.34%> (+6.57%)`	`19 <17> (+17)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f40469...c512cd7. Read the comment docs.

sharajava

I'm not sure if hooks are really needed by users. Can you share some ideas about hook use cases?

And also, the hook mechanism here is doing something just like FailureHandler. I think we can replace FailureHandler with TransactionHook after some enhancement on this.

sharajava · 2019-03-06T07:28:06Z

tm/src/main/java/com/alibaba/fescar/tm/api/TransactionalTemplate.java

+            }
+            // 4. everything is fine, commit.
+            try {
+                triggerBeforeCommit();


Exception should be properly handled. I don't think users would like to fail committing by their hook.

fixed it in next commit

the hook help us to add more function in the transaction life cycle
like
1.trace the transaction execute status
2. customer logger to help they find the bad transaction
3.some case we need to do something when the commit all success

sharajava · 2019-03-06T07:29:03Z

tm/src/main/java/com/alibaba/fescar/tm/api/transaction/TransactionHook.java

+ * @author guoyao
+ * @date 2019/3/4
+ */
+public interface TransactionHook {


As a transaction hook, why not also including before/after rollback?

good idea .i will add it

…ure_add_hook_0304

slievrly · 2019-03-06T03:34:22Z

tm/src/main/java/com/alibaba/fescar/tm/api/TransactionalTemplate.java


            }
-
+            try {
+                triggerAfterCommit();


merge after tx.commit()？

good idea ,done

jovany-wang

I left some comments about code style.

jovany-wang · 2019-03-07T03:23:51Z

tm/src/main/java/com/alibaba/fescar/tm/api/TransactionalTemplate.java

+import com.alibaba.fescar.tm.api.transaction.TransactionHookManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+


remove this line.

already format

jovany-wang · 2019-03-07T03:45:54Z

tm/src/main/java/com/alibaba/fescar/tm/api/transaction/TransactionHookManager.java

+ */
+package com.alibaba.fescar.tm.api.transaction;
+
+import java.util.*;


Don't use *

jovany-wang · 2019-03-07T03:46:12Z

tm/src/main/java/com/alibaba/fescar/tm/api/transaction/TransactionHookManager.java

+        if (transactionHook == null) {
+            throw new NullPointerException("transactionHook must not be null");
+        }
+        List<TransactionHook> transactionHooks=hooksLocal.get();


the spaces around =

jovany-wang · 2019-03-07T03:46:23Z

tm/src/test/java/com/alibaba/fescar/tm/api/TransactionTemplateTest.java

+ */
+public class TransactionTemplateTest {
+
+    private static final String DEFAULT_XID="123456789";


jovany-wang · 2019-03-07T12:36:31Z

tm/src/main/java/com/alibaba/fescar/tm/api/transaction/TransactionHookManager.java

@@ -26,7 +26,7 @@
 */
 public final class TransactionHookManager {

-    private static final ThreadLocal<List<TransactionHook>> hooksLocal=new ThreadLocal<>();
+    private static final ThreadLocal<List<TransactionHook>> hooksLocal = new ThreadLocal<>();


Let's call a static final member as uppercase:

private static final ThreadLocal<List<TransactionHook>> LOCAL_HOOKS = new ThreadLocal<>();

why not use spi to load the transaction hook instead of a thread local holder?

…ure_add_hook_0304

slievrly · 2019-03-15T04:12:31Z

@jovany-wang

tm/src/main/java/com/alibaba/fescar/tm/api/transaction/TransactionHookAdapter.java

jovany-wang

Thanks, LGTM!

github-ygy added 3 commits March 4, 2019 23:11

add hooks in the tx

6e87c02

add test for hook

e5a22de

Merge branch 'develop' of https://github.com/alibaba/fescar into feat…

be0aacc

…ure_add_hook_0304

tomiok approved these changes Mar 6, 2019

View reviewed changes

sharajava requested changes Mar 6, 2019

View reviewed changes

github-ygy added 2 commits March 6, 2019 19:20

add rollback hook in transaction

d5b9e2c

Merge branch 'develop' of https://github.com/alibaba/fescar into feat…

9566b6f

…ure_add_hook_0304

slievrly reviewed Mar 6, 2019

View reviewed changes

github-ygy added 3 commits March 6, 2019 20:34

remove the unnecessary exception

b787ff6

updated mockito version

43b8051

change hook set to list

659ae29

jovany-wang suggested changes Mar 7, 2019

View reviewed changes

github-ygy added 2 commits March 7, 2019 19:12

format code

6505e28

add spaces around =

a26df0c

jovany-wang suggested changes Mar 7, 2019

View reviewed changes

optimize code style

5aa2782

sharajava approved these changes Mar 8, 2019

View reviewed changes

skyesx mentioned this pull request Mar 8, 2019

[discuss] #525 tx hook should every catch or all catch #557

Open

github-ygy added 2 commits March 15, 2019 11:46

Merge branch 'develop' of https://github.com/alibaba/fescar into feat…

0c2bb6c

…ure_add_hook_0304

change exception to every catch

c512cd7

slievrly approved these changes Mar 15, 2019

View reviewed changes

tm/src/main/java/com/alibaba/fescar/tm/api/transaction/TransactionHookAdapter.java Show resolved Hide resolved

jovany-wang approved these changes Mar 15, 2019

View reviewed changes

slievrly merged commit d9d4793 into apache:develop Mar 15, 2019

fescar-robot mentioned this pull request Mar 15, 2019

[WeeklyReport] Weekly report for FESCAR 2019-3-8 to 2019-3-15 #605

Closed

wangliang181230 added this to the 0.3.* milestone Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] add transaction hook #525

[Feature] add transaction hook #525

github-ygy commented Mar 5, 2019 •

edited

Loading

codecov-io commented Mar 5, 2019 •

edited

Loading

sharajava left a comment

sharajava Mar 6, 2019

github-ygy Mar 6, 2019

github-ygy Mar 6, 2019

sharajava Mar 6, 2019

github-ygy Mar 6, 2019

slievrly Mar 6, 2019

github-ygy Mar 6, 2019

jovany-wang left a comment

jovany-wang Mar 7, 2019

github-ygy Mar 7, 2019

jovany-wang Mar 7, 2019

github-ygy Mar 7, 2019

jovany-wang Mar 7, 2019

github-ygy Mar 7, 2019

jovany-wang Mar 7, 2019

github-ygy Mar 7, 2019

jovany-wang Mar 7, 2019

hoswey Dec 17, 2020

slievrly commented Mar 15, 2019

jovany-wang left a comment

[Feature] add transaction hook #525

[Feature] add transaction hook #525

Conversation

github-ygy commented Mar 5, 2019 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Mar 5, 2019 • edited Loading

Codecov Report

sharajava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jovany-wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slievrly commented Mar 15, 2019

jovany-wang left a comment

Choose a reason for hiding this comment

github-ygy commented Mar 5, 2019 •

edited

Loading

codecov-io commented Mar 5, 2019 •

edited

Loading