Prevent NetworkException from causing a double-write #982

rogermichoud · 2017-11-03T23:13:18Z

The Runtime has a settable timeout for how long it will wait for
an unavailable system. If we are not able to progress (due to NetworkExcpetion)
during the timeout, the handler provided by the client (or default handler,
that shutdown the Runtime) will be triggered.

Fixes #974

no2chem

Good start, but the handler should decide what to do, not hardcoded timeouts.

So I would remove all the timeout fields. When a NetworkException is thrown, execute the handler. If the handler throws an exception, re-throw it, otherwise continue retrying forever.

no2chem · 2017-11-07T17:21:15Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

+                    } catch (InterruptedException e) {
+                        log.warn("Interrupted Exception in layout helper.", e);
+                    }
+                    log.warn("System seems unavailable");


Correct format is getCurrentLayout: x

rogermichoud · 2017-11-13T19:20:17Z

@no2chem
The problem is that there are many threads that are executing AbstractView layoutHelper. In order to implement a timeout crafted in the handler, I would need to use ThreadLocals and add variables to my default implementation of the handler in CorfuRuntime. Doesn't look that clean to me.

What I can do instead is pass the timeout parameter with the handler. Like this, if somebody wants to register a handler that fires directly, he can pass a -1 for the timeout.

no2chem · 2017-11-13T19:24:42Z

@rogermichoud I'm not sure why threadlocals are necessary.
Timeouts should still be decided globally on a per-request basis. The handler should get the NetworkException, and decide what to do; this doesn't need to be on a per-thread basis, in fact, it shouldn't be --- it should be on a per-endpoint basis.

By default, there should be no handler (i.e., the handler should always return).

rogermichoud · 2017-11-13T21:01:42Z

@no2chem
You want the default behavior to hang forever? I thought the consensus was having the default behavior to shutdown the runtime after a timeout (SystemUnavailable state).

no2chem · 2017-11-13T21:25:37Z

Yes, the default behavior should be to (retry, not hang) forever. If there is an application which has a specific behavior requirement (e.g., mp, they may specify it using the handler).

no2chem

Great - much better - mainly see the comment about UnknownResultException. We definitely don't want the PR to introduce data consistency issues due to trim (but it's ok not to handle it now).

no2chem · 2017-11-29T22:55:18Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

@@ -97,9 +113,23 @@ public Layout getCurrentLayout() {
                    log.warn("Got a wrong epoch exception, updating epoch to {} and "
                            + "invalidate view", we.getCorrectEpoch());
                    runtime.invalidateLayout();
+                } else if (re instanceof NetworkException) {
+                    log.warn("layoutHelper: System seems unavailable");


Print the exception too:

log.warn("layoutHelper: System seems unavailable", re);

no2chem · 2017-11-29T22:56:52Z

test/src/test/java/org/corfudb/runtime/CorfuRuntimeTest.java

+        class TimeoutHandler {
+            CorfuRuntime rt;
+            long maxTimeout;
+            ThreadLocal<Long> localTimeStart = new ThreadLocal<>();


Do we want this on a per-thread basis?

Well this is an example, so I guess there is some freedom on how to implement it. I also think different threads could be interacting with different routers. If one thread is only reading, its queries will only go to the chain tail, and it will reset the timer each time. This could mask a problem with writes that could never go in.

no2chem · 2017-11-29T22:58:25Z

runtime/src/main/java/org/corfudb/runtime/view/AddressSpaceView.java

+        } catch (TrimmedException te) {
+            // We cannot know if the write went through or not
+            // This will rewrite
+            throw new OverwriteException();


I think we should throw a new exception type, "UnknownResultException"
An overwrite exception will cause a data inconsistency issue, which will not be nice....

For a transaction, we should abort. For a object, we should recover from a checkpoint(?) - but we still won't know if the operation committed or not. We can deal with handling the exception in a separate PR, but we absolutely don't want to introduce data consistency issues.

I want to throw your UnrecoverableCorfuException. That will just create a dependency on your PR. Which is fine. The UnreceoverableCorfruExcpetion should go in pretty soon.

Sounds good.. It's UnrecoverableCorfuError now :). Can you create a dependency on ZenHub so we can track it?

corfu-repo · 2017-12-09T03:41:26Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

@@ -93,14 +110,38 @@ public Layout getCurrentLayout() {
                    log.warn("Got a wrong epoch exception, updating epoch to {} and "
                            + "invalidate view", we.getCorrectEpoch());
                    runtime.invalidateLayout();
+                } else if (re instanceof NetworkException) {


Replace the usage of the "instanceof" operator by a catch block.

corfu-repo · 2017-12-09T03:41:26Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

                } else {
                    throw re;
                }
+                if (rethrowAllExceptions) {
+                    throw new RuntimeException(re);


Define and throw a dedicated exception instead of using a generic one.

corfu-repo · 2017-12-09T03:41:26Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

+                if (ex.getCause() instanceof SystemUnavailableException) {
+                    throw (SystemUnavailableException) ex.getCause();
+                }
+
                runtime.invalidateLayout();
                Utils.sleepUninterruptibly(runtime.retryRate * 1000);


Cast one of the operands of this multiplication operation to a "long".

corfu-repo · 2017-12-09T03:41:27Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

     * @return The return value of the function.
     */
    public <T, A extends RuntimeException, B extends RuntimeException, C extends RuntimeException,
            D extends RuntimeException> T layoutHelper(LayoutFunction<Layout, T, A, B, C, D>
-                                                                          function)
+                                                                          function,
+                                                       boolean rethrowAllExceptions)
            throws A, B, C, D {


Remove the declaration of thrown exception 'A' which is a runtime exception.
Remove the declaration of thrown exception 'B' which is a runtime exception.
Remove the declaration of thrown exception 'C' which is a runtime exception.
Remove the declaration of thrown exception 'D' which is a runtime exception.

corfu-repo · 2017-12-09T03:41:27Z

runtime/src/main/java/org/corfudb/runtime/CorfuRuntime.java

+     * test/src/test/java/org/corfudb/runtime/CorfuRuntimeTest.java
+     *
+     */
+    public Runnable beforeRpcHandler = () -> {};


Either remove or fill this block of code.
Make beforeRpcHandler a static final constant or non-public and provide accessors if needed.

corfu-repo · 2017-12-09T03:41:28Z

runtime/src/main/java/org/corfudb/runtime/CorfuRuntime.java

+     *
+     */
+    public Runnable beforeRpcHandler = () -> {};
+    public Runnable systemDownHandler = () -> {};


Either remove or fill this block of code.
Make systemDownHandler a static final constant or non-public and provide accessors if needed.

corfu-repo · 2017-12-09T03:41:28Z

runtime/src/main/java/org/corfudb/runtime/view/AddressSpaceView.java

+            } catch (RuntimeException re) {
+                // These two exceptions are pass through. Both of them are already too late for trying
+                // to validate the state of the write, we know that it didn't went through.
+                if (re instanceof SystemUnavailableException || re instanceof OverwriteException) {


Replace the usage of the "instanceof" operator by a catch block.
Replace the usage of the "instanceof" operator by a catch block.

corfu-repo · 2017-12-09T03:41:29Z

runtime/src/main/java/org/corfudb/runtime/view/AddressSpaceView.java

+                if (re instanceof SystemUnavailableException || re instanceof OverwriteException) {
+                    throw re;
+                }
+                re.printStackTrace();


Use a logger to log this exception.

Remove this printStackTrace

corfu-repo · 2017-12-09T03:41:29Z

test/src/test/java/org/corfudb/integration/ServerRestartIT.java

+        },CLIENT_DELAY_POST_SHUTDOWN, TimeUnit.MILLISECONDS);
+        offline.shutdown();
+
+        Thread.sleep(CORFU_SERVER_DOWN_TIME);


Remove this use of "Thread.sleep()".

corfu-repo · 2017-12-09T03:41:30Z

SonarQube analysis reported 16 issues

8 major
8 minor

Watch the comments in this conversation to review them.

1 extra issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

CorfuRuntime.java#L586: Either re-interrupt this method or rethrow the "InterruptedException".

coveralls · 2017-12-09T03:41:52Z

Coverage increased (+0.06%) to 70.702% when pulling 149e859 on systemUnavailable into 0174570 on master.

corfudb-performance · 2017-12-09T10:17:08Z

Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 149e859.

*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 10 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 10 threads, Disk mode

An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at:
Pull Request #982 Graphs

no2chem

Looks good once you remove the printStackTrace

rogermichoud · 2017-12-11T19:58:31Z

@no2chem I had to do some tweaks to tests and also be sure that the custom handler would get out of fetchLayout as well if we cannot reach any layout. Should not impact the default path.

Also I wanted to be sure we handle both cases correctly for custom handlers:
1. We cannot write because one (or more) server of the logunit chain is not reachable but we can get a layout (some or all layout servers are up).
2. We cannot write and we cannot have a layout. In that case, we are stuck in fetchLayout.

if the changes are ok for you, we can merge.

rogermichoud · 2017-12-11T20:02:06Z

@no2chem
Also removed the printStackTrace

no2chem · 2017-12-11T20:04:56Z

Hm - Also, weren't you going to have SystemUnavailableException extend UnrecoverableCorfuError?

rogermichoud · 2017-12-11T20:09:25Z

Well, I wasn't but I will :-)

no2chem

Just 2 small nits left...

no2chem · 2017-12-11T21:56:12Z

runtime/src/main/java/org/corfudb/runtime/view/AbstractView.java

@@ -45,6 +45,8 @@ public Layout getCurrentLayout() {
        while (true) {
            try {
                return runtime.layout.get();
+            } catch (SystemUnavailableException sue) {


This shouldn't be necessary, the error will not be caught by the exception catch block below

no2chem · 2017-12-11T21:58:42Z

runtime/src/main/java/org/corfudb/runtime/exceptions/SystemUnavailableException.java

+/**
+ * Created by rmichoud on 10/31/17.
+ */
+public class SystemUnavailableException extends UnrecoverableCorfuError {


SystemUnavailableException -> SystemUnavailableError. Also you can move it to the Unrecoverable package.

NetworkExcpetion within the layoutHelper function will call the handler. The default handler will do nothing and just let layoutHelper retry the request. For writes(Transaction and single update), NetworkExceptions are passed to the view layer (StreamsView and BackPointerView). After a NetworkException, the layer will force a read to see if we persisted the record or not. If it was persisted, the request return the correct value. If not, the retry logic is implemented in the view.

no2chem

Excellent, thanks!

NetworkExcpetion within the layoutHelper function will call the handler. The default handler will do nothing and just let layoutHelper retry the request. For writes(Transaction and single update), NetworkExceptions are passed to the view layer (StreamsView and BackPointerView). After a NetworkException, the layer will force a read to see if we persisted the record or not. If it was persisted, the request return the correct value. If not, the retry logic is implemented in the view.

no2chem requested changes Nov 7, 2017

View reviewed changes

rogermichoud force-pushed the systemUnavailable branch from 64cf8cf to aa8da61 Compare November 14, 2017 07:09

rogermichoud mentioned this pull request Nov 15, 2017

Ignore Network Exceptions #1001

Closed

Maithem self-requested a review November 20, 2017 21:39

rogermichoud force-pushed the systemUnavailable branch from aa8da61 to e0d0dd2 Compare November 21, 2017 19:37

rogermichoud force-pushed the systemUnavailable branch 2 times, most recently from 48daa76 to 72201ff Compare November 28, 2017 19:43

rogermichoud changed the title ~~[WIP] A handler is provided in case of system unavailable.~~ A handler is provided in case of system unavailable. Nov 28, 2017

rogermichoud force-pushed the systemUnavailable branch 2 times, most recently from 02ba039 to b2a7b9f Compare November 29, 2017 01:47

no2chem requested changes Nov 29, 2017

View reviewed changes

no2chem changed the title ~~A handler is provided in case of system unavailable.~~ Prevent NetworkException from causing a double-write Nov 29, 2017

no2chem added this to the 0.2.0 milestone Dec 4, 2017

no2chem added the bug label Dec 4, 2017

zalokhan mentioned this pull request Dec 6, 2017

Fix set router epoch on runtime invalidate. #1046

Merged

4 tasks

rogermichoud force-pushed the systemUnavailable branch from b2a7b9f to 149e859 Compare December 9, 2017 03:16

corfu-repo reviewed Dec 9, 2017

View reviewed changes

no2chem requested changes Dec 11, 2017

View reviewed changes

rogermichoud force-pushed the systemUnavailable branch from 149e859 to 030e579 Compare December 11, 2017 20:01

rogermichoud force-pushed the systemUnavailable branch 2 times, most recently from ceed3a4 to 8e2758b Compare December 11, 2017 20:16

no2chem requested changes Dec 11, 2017

View reviewed changes

no2chem reviewed Dec 11, 2017

View reviewed changes

rogermichoud force-pushed the systemUnavailable branch from 8e2758b to 107d0fa Compare December 11, 2017 22:39

no2chem approved these changes Dec 11, 2017

View reviewed changes

no2chem merged commit 0c92e3c into master Dec 11, 2017

Maithem deleted the systemUnavailable branch December 15, 2017 10:47

Prevent NetworkException from causing a double-write #982

Prevent NetworkException from causing a double-write #982

Conversation

rogermichoud commented Nov 3, 2017 • edited by no2chem Loading

no2chem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogermichoud commented Nov 13, 2017

no2chem commented Nov 13, 2017

rogermichoud commented Nov 13, 2017

no2chem commented Nov 13, 2017

no2chem 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

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

corfu-repo commented Dec 9, 2017

1 extra issue

coveralls commented Dec 9, 2017

corfudb-performance commented Dec 9, 2017

no2chem left a comment

Choose a reason for hiding this comment

rogermichoud commented Dec 11, 2017

rogermichoud commented Dec 11, 2017

no2chem commented Dec 11, 2017

rogermichoud commented Dec 11, 2017

no2chem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

no2chem left a comment

Choose a reason for hiding this comment

rogermichoud commented Nov 3, 2017 •

edited by no2chem

Loading