SCB-224 retry sub-transaction on failure by eric-lee-ltk · Pull Request #138 · apache/servicecomb-pack

eric-lee-ltk · 2018-02-24T02:20:43Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

This PR is based on #117

WillemJiang · 2018-02-24T15:33:14Z

@eric-lee-ltk There are some build error in you commit, could you take a look at it?

coveralls · 2018-02-27T10:06:04Z

Coverage increased (+0.3%) to 95.361% when pulling 436296b on eric-lee-ltk:SCB-224 into aff2719 on apache:master.

WillemJiang · 2018-03-12T13:30:17Z

-    aspect.advise(joinPoint, compensable);
+    try {
+      aspect.advise(joinPoint, compensable);
+    } catch (Exception e) {


We don't need to catch the exception here, as the JUnit test failed when the exception is throw.

WillemJiang · 2018-03-12T13:33:06Z

+        expectFailing(InterruptedException.class);
+      } catch (InterruptedException ignored) {
+      } catch (Throwable throwable) {
+        fail("unexpected exception throw: " + throwable);


I'm not sure if the fail can work, as the exception is thrown in another thread. Maybe you need to run a test for it.

WillemJiang · 2018-03-12T13:39:55Z

 @Aspect
 public class TransactionAspect {
-  private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());


It's common practice to use upper case for the static constant.

WillemJiang · 2018-03-12T13:40:20Z

+package org.apache.servicecomb.saga.omega.transaction;
+
+public class RecoveryPolicyFactory {
+  private static final RecoveryPolicy defaultRecovery = new DefaultRecovery();


It's common practice to use upper case for the static constant.

WillemJiang · 2018-03-12T13:41:06Z

+
+  private static final RecoveryPolicy forwardRecovery = new ForwardRecovery();
+
+  static RecoveryPolicy getRecoveryPolicy(int retries) {


How can you decide the RecoveryPolicy by using the retries parameter.

Which parameter is more proper?

It depends on the RecoveryPolicy that you have, maybe you need to add some comments for the DefaultRecovery.

WillemJiang · 2018-03-12T13:43:34Z

+
+          log.warn("Retrying sub tx failed, global tx id: {}, local tx id: {}, method: {}, remains: {}",
+              context.globalTxId(), context.localTxId(), method.toString(), remains);
+          Thread.sleep(compensable.retryDelayInMilliseconds());


It may cause some performance issue by using Thread.sleep here.
we may need to consider to use scheduler for it.

Scheduler requires to implement the Callable interface to retrieve the return value. However, we need to construct a new instance for every execution to pass the paramenters. The times of execution is also hard to control. I guess this also introduce performance penalty.

WillemJiang · 2018-03-12T13:44:38Z

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DefaultRecovery implements RecoveryPolicy {


Is it a backward recovery?

Not exactly. As the compensation of backward recovery is handled asynchronously, this place is only used to execute bussiness logic once.

Signed-off-by: Eric Lee <dagang.li@huawei.com>

Signed-off-by: Eric Lee <eric.lee.ltk@gmail.com>

WillemJiang · 2018-04-24T00:22:20Z

+      + "t.type, t.compensationMethod, t.payloads "
      + ") FROM TxEvent t "
-      + "WHERE t.globalTxId = ?1 AND t.type = ?2")
+      + "WHERE t.globalTxId = ?1 AND t.type = ?2 "


It's a litter bit complicated, as we have retry option, the retry failed event is not same with the abort event. It should be easy if we introduce retry failed event.

WillemJiang · 2018-04-28T09:13:46Z

Merged the patch into master branch.

eric-lee-ltk force-pushed the SCB-224 branch 2 times, most recently from 3d748fd to c26feb1 Compare February 27, 2018 09:46

eric-lee-ltk force-pushed the SCB-224 branch 3 times, most recently from f516204 to c1d96b2 Compare March 12, 2018 12:03

WillemJiang reviewed Mar 12, 2018

View reviewed changes

eric-lee-ltk force-pushed the SCB-224 branch from 309d4f8 to 390d5ac Compare March 13, 2018 10:19

eric-lee-ltk force-pushed the SCB-224 branch from 6c34c4f to 6930930 Compare April 4, 2018 08:19

eric-lee-ltk force-pushed the SCB-224 branch from 6930930 to c3e7396 Compare April 17, 2018 08:17

eric-lee-ltk changed the title ~~[WIP] SCB-224 retry sub-transaction on failure~~ SCB-224 retry sub-transaction on failure Apr 17, 2018

eric-lee-ltk force-pushed the SCB-224 branch 3 times, most recently from f627755 to b8a1163 Compare April 17, 2018 10:07

zhang2014 added 5 commits April 22, 2018 21:28

SCB-224: alpha support retries

067b0c1

SCB-224: omega support retries

e3609f7

SCB-224 alpha support retries event

4f12934

SCB-224 omega support retries event

1061b0d

SCB-224 alpha:support retries use command

47fb5ee

eric-lee-ltk force-pushed the SCB-224 branch from 82be53a to 7157bd4 Compare April 22, 2018 13:57

eric-lee-ltk and others added 2 commits April 24, 2018 16:18

SCB-224 retry synchronously

15fac01

Signed-off-by: Eric Lee <dagang.li@huawei.com>

SCB-224 resolved rebase conflicts

436296b

Signed-off-by: Eric Lee <eric.lee.ltk@gmail.com>

eric-lee-ltk force-pushed the SCB-224 branch from 62f190b to 436296b Compare April 24, 2018 08:19

WillemJiang reviewed Apr 26, 2018

View reviewed changes

WillemJiang approved these changes Apr 28, 2018

View reviewed changes

WillemJiang merged commit ce71ab7 into apache:master Apr 28, 2018

eric-lee-ltk deleted the SCB-224 branch April 28, 2018 09:35

WillemJiang mentioned this pull request May 29, 2018

SCB-224 support retry sub-transaction #117

Closed

6 tasks


		private static final RecoveryPolicy forwardRecovery = new ForwardRecovery();

		static RecoveryPolicy getRecoveryPolicy(int retries) {

Conversation

eric-lee-ltk commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WillemJiang commented Feb 24, 2018

Uh oh!

coveralls commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillemJiang commented Apr 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eric-lee-ltk commented Feb 24, 2018 •

edited

Loading

coveralls commented Feb 27, 2018 •

edited

Loading