SCB-227 stop sub tx from running when global tx failed #114

eric-lee-ltk · 2018-01-12T11:08:32Z

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.

coveralls · 2018-01-12T11:23:14Z

Changes Unknown when pulling dfb53f2 on eric-lee-ltk:SCB-227 into ** on apache:master**.

coveralls · 2018-01-12T15:49:35Z

Changes Unknown when pulling b95ae53 on eric-lee-ltk:SCB-227 into ** on apache:master**.

coveralls · 2018-01-13T22:52:18Z

Coverage increased (+0.1%) to 95.416% when pulling cd867f9 on eric-lee-ltk:SCB-227 into 9dd6a63 on apache:master.

coveralls · 2018-01-14T16:20:16Z

Coverage decreased (-0.3%) to 94.901% when pulling e9128a1 on eric-lee-ltk:SCB-227 into 5b2b59a on apache:master.

seanyinx · 2018-01-15T01:00:07Z

alpha/alpha-core/src/main/java/org/apache/servicecomb/saga/alpha/core/TxConsistentService.java

@@ -100,4 +106,8 @@ private boolean isGlobalTxAborted(TxEvent event) {
  private boolean isTxEndedEvent(TxEvent event) {
    return TxEndedEvent.name().equals(event.type());
  }
+
+  private boolean isInvalidTxStarted(TxEvent event) {


probably inlining this method is more readable

seanyinx · 2018-01-15T01:02:41Z

...ansaction/src/main/java/org/apache/servicecomb/saga/omega/transaction/TransactionAspect.java

+    if (!ok) {
+      LOG.info("Skipped transaction {} due to abort.", context.globalTxId());
+      context.setLocalTxId(localTxId);
+      return null;


not a good idea to return null, since user doesn't expect it

seanyinx · 2018-01-15T01:06:37Z

pack-contracts/pack-contract-grpc/src/main/proto/GrpcTxEvent.proto

@@ -33,6 +33,7 @@ message GrpcServiceConfig {
 }

 message GrpcAck {
+  bool valid = 1;


valid doesn't seem to be a proper name, because the transaction is indeed valid. probably just aborted?

seanyinx · 2018-01-15T01:09:23Z

.../src/main/java/org/apache/servicecomb/saga/omega/connector/grpc/GrpcClientMessageSender.java

-    blockingEventService.onTxEvent(convertEvent(event));
+  public boolean send(TxEvent event) {
+    GrpcAck grpcAck = blockingEventService.onTxEvent(convertEvent(event));
+    return grpcAck.getValid();


better to return a proper AlphaResponse class instead of boolean

coveralls · 2018-01-15T03:05:52Z

Coverage increased (+0.08%) to 95.306% when pulling 3891afc on eric-lee-ltk:SCB-227 into 5b2b59a on apache:master.

seanyinx · 2018-01-15T04:32:59Z

...ansaction/src/main/java/org/apache/servicecomb/saga/omega/transaction/TransactionAspect.java

+    if (response.aborted()) {
+      String abortedLocalTxId = context.localTxId();
+      context.setLocalTxId(localTxId);
+      throw new IllegalStateException("Abort local sub transaction " + abortedLocalTxId +


shouldn't this throw transaction exception?

seanyinx · 2018-01-15T04:34:18Z

.../java/org/apache/servicecomb/saga/omega/connector/grpc/LoadBalancedClusterMessageSender.java

@@ -123,14 +124,15 @@ public void close() {
  }

  @Override
-  public void send(TxEvent event) {
+  public AlphaResponse send(TxEvent event) {
+    AlphaResponse response = new AlphaResponse(false);


what behavior is expected from omega, if this response returns?

coveralls · 2018-01-15T07:16:13Z

Coverage increased (+0.5%) to 95.329% when pulling 0bc5598 on eric-lee-ltk:SCB-227 into 7bfb6a9 on apache:master.

coveralls · 2018-01-15T07:28:20Z

Coverage increased (+0.4%) to 95.28% when pulling 0550e93 on eric-lee-ltk:SCB-227 into 7bfb6a9 on apache:master.

seanyinx · 2018-01-15T07:24:25Z

...ansaction/src/main/java/org/apache/servicecomb/saga/omega/transaction/TransactionAspect.java

+    if (response.aborted()) {
+      String abortedLocalTxId = context.localTxId();
+      context.setLocalTxId(localTxId);
+      throw new OmegaTxAbortedException("Abort local sub transaction " + abortedLocalTxId +


better to throw TransactionException defined in spring, because user is not going to catch OmegaTxAbortedException.

using the spring's TransactionException will introduce spring dependency in our omega transaction module, is it all right to do so?

seanyinx · 2018-01-15T07:28:04Z

.../java/org/apache/servicecomb/saga/omega/connector/grpc/LoadBalancedClusterMessageSender.java

@@ -124,14 +125,15 @@ public void close() {
  }

  @Override
-  public void send(TxEvent event) {
+  public AlphaResponse send(TxEvent event) {
+    AlphaResponse response = null;


have to think about when this response will be returned, and the consequence of returning this response under such a scenario. apparently it's only possible when this thread is interrupted and throwing an exception is a better idea in such case.

seanyinx · 2018-01-15T07:30:50Z

...ction/src/test/java/org/apache/servicecomb/saga/omega/transaction/TransactionAspectTest.java

+    } catch (OmegaTxAbortedException e) {
+      assertThat(e.getMessage().contains("Abort local sub transaction"), is(true));
+    } catch (Throwable throwable) {
+      expectFailing(OmegaTxAbortedException.class);


this is not necessary, since test will fail on uncaught exception

seanyinx · 2018-01-15T07:33:51Z

.../java/org/apache/servicecomb/saga/omega/connector/grpc/LoadBalancedClusterMessageSender.java

          } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
          }
+          return new AlphaResponse(true);


same as above

seanyinx · 2018-01-15T07:34:50Z

...a-server/src/main/java/org/apache/servicecomb/saga/alpha/server/GrpcTxEventEndpointImpl.java

@@ -85,7 +87,12 @@ public void onTxEvent(GrpcTxEvent message, StreamObserver<GrpcAck> responseObser
        message.getPayloads().toByteArray()
    ));

-    responseObserver.onNext(ACK);
+    if (ok) {


this can be simplified into one line

coveralls · 2018-01-15T11:03:05Z

Coverage increased (+0.07%) to 95.367% when pulling 264c32d on eric-lee-ltk:SCB-227 into d6e9c48 on apache:master.

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

coveralls · 2018-01-15T11:45:20Z

Coverage increased (+0.07%) to 95.186% when pulling 1d23495 on eric-lee-ltk:SCB-227 into 5f1dce3 on apache:master.

seanyinx · 2018-01-15T13:22:22Z

...a/org/apache/servicecomb/saga/omega/connector/grpc/LoadBalancedClusterMessageSenderTest.java

+      }
+    });
+    thread.start();
+    thread.interrupt();


need to join here to wait for thread exit

seanyinx · 2018-01-15T13:24:27Z

...ansaction/src/main/java/org/apache/servicecomb/saga/omega/transaction/TransactionAspect.java

+    if (response.aborted()) {
+      String abortedLocalTxId = context.localTxId();
+      context.setLocalTxId(localTxId);
+      throw new InvalidTransactionException("Abort local sub transaction " + abortedLocalTxId +


local transaction is sub transaction, "Aborted sub transaction... because global transaction ..."

seanyinx · 2018-01-15T13:30:37Z

.../java/org/apache/servicecomb/saga/omega/connector/grpc/LoadBalancedClusterMessageSender.java

            Thread.currentThread().interrupt();
          }
-        }));
+          throw new OmegaException("Failed to send event " + event + " due to interruption");


preferably to move this line to catch clause and pass interrupted exception to its constructor

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

coveralls · 2018-01-15T15:21:13Z

Coverage decreased (-0.2%) to 94.891% when pulling 366edd4 on eric-lee-ltk:SCB-227 into 5f1dce3 on apache:master.

eric-lee-ltk force-pushed the SCB-227 branch from dfb53f2 to b95ae53 Compare January 12, 2018 15:33

eric-lee-ltk force-pushed the SCB-227 branch from b95ae53 to cd867f9 Compare January 13, 2018 22:35

eric-lee-ltk force-pushed the SCB-227 branch from cd867f9 to e9128a1 Compare January 14, 2018 16:05

seanyinx reviewed Jan 15, 2018

View reviewed changes

eric-lee-ltk force-pushed the SCB-227 branch from 0df79fa to 3891afc Compare January 15, 2018 02:51

seanyinx reviewed Jan 15, 2018

View reviewed changes

eric-lee-ltk force-pushed the SCB-227 branch 3 times, most recently from 0bc5598 to 0550e93 Compare January 15, 2018 07:12

seanyinx reviewed Jan 15, 2018

View reviewed changes

eric-lee-ltk force-pushed the SCB-227 branch from 012c6a1 to 264c32d Compare January 15, 2018 10:30

eric-lee-ltk added 5 commits January 15, 2018 19:28

SCB-227 stop sub tx from running when global tx failed

0a292a9

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

SCB-227 proper namings for aborted status

231a95a

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

SCB-227 add OmegaTxAbortedException

a7b4540

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

SCB-227 throw OmegaException when sending get interrupted

9263f61

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

SCB-227 use InvalidTransactionException instead of custom exception

1d23495

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

eric-lee-ltk force-pushed the SCB-227 branch from 264c32d to 1d23495 Compare January 15, 2018 11:31

seanyinx reviewed Jan 15, 2018

View reviewed changes

SCB-227 minor fix

366edd4

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

eric-lee-ltk force-pushed the SCB-227 branch from b1d616a to 366edd4 Compare January 15, 2018 15:06

seanyinx merged commit 9643ad8 into apache:master Jan 16, 2018

eric-lee-ltk deleted the SCB-227 branch January 16, 2018 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCB-227 stop sub tx from running when global tx failed #114

SCB-227 stop sub tx from running when global tx failed #114

eric-lee-ltk commented Jan 12, 2018 •

edited

coveralls commented Jan 12, 2018

coveralls commented Jan 12, 2018

coveralls commented Jan 13, 2018

coveralls commented Jan 14, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

coveralls commented Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018

seanyinx Jan 15, 2018

eric-lee-ltk Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

seanyinx Jan 15, 2018

coveralls commented Jan 15, 2018

SCB-227 stop sub tx from running when global tx failed #114

SCB-227 stop sub tx from running when global tx failed #114

Conversation

eric-lee-ltk commented Jan 12, 2018 • edited

coveralls commented Jan 12, 2018

coveralls commented Jan 12, 2018

coveralls commented Jan 13, 2018

coveralls commented Jan 14, 2018

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

coveralls commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018

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

coveralls commented Jan 15, 2018

coveralls commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2018

eric-lee-ltk commented Jan 12, 2018 •

edited