Handle disconnection/reconnection of worker and help it recover interrupted replicates #234

zguesmi · 2019-03-15T17:07:01Z

Handle different cases when worker disconnects and tries to recover from it
Introduce ReplicateSupplyService that handles replicateDemand requests coming from workers
Introduce RECOVERING replicateStatus: every transition from this task is permitted, transitions to this task come only from "active" (not ..._failed) statuses.

…-core into reconnect

Ugo

Very nice work, the ReplicateSupplyService is very useful, however there are a few points I noticed in the code to check.

Ugo · 2019-03-18T13:32:05Z

src/main/java/com/iexec/core/replicate/Replicate.java

+            }
+        }
+
+        return null;


return an optional instead of returning a null.

Would be better yes

Ugo · 2019-03-18T13:35:15Z

src/main/java/com/iexec/core/chain/SignatureService.java

@@ -38,20 +40,20 @@ String computeAuthorizationHash(String workerWallet, String chainTaskId, String
        return Numeric.toHexString(Hash.sha3(res));
    }

-    public ContributionAuthorization createAuthorization(String workerWallet, String chainTaskId, boolean isTrustedExecution) {
+    public Optional<ContributionAuthorization> createAuthorization(String workerWallet, String chainTaskId, boolean isTrustedExecution) {


Why do we use an optional here? In which case will it be Optional.empty()?

sorry I was lost in the refactoring

Ugo · 2019-03-18T13:35:54Z

src/main/java/com/iexec/core/replicate/ReplicateSupplyService.java

+            if (recoveryAction == null) continue;
+
+            // generate contribution authorization
+            Optional<ContributionAuthorization> authorization = signatureService.createAuthorization(


The optional here is not needed.

Ugo · 2019-03-18T14:00:38Z

src/main/java/com/iexec/core/replicate/ReplicateSupplyService.java

+    }
+
+    public RecoveryAction getAppropriateRecoveryAction(Task task, Replicate replicate, long blockNumber) {
+        ChainReceipt chainReceipt = new ChainReceipt(blockNumber, "");


The chainReceipt object is not needed.

It is actually, because in the case where ReplicateStatus is CONTRIBUTING and the worker contributes onchain without notifying the core should update the status to CONTRIBUTED and tell the worker to WAIT or REVEAL. So when updating the status to CONTRIBUTED we check if it's correct onchain, thus, we need the chainReceipt with the blocknumber.

Yes, I can see it is used in methods recoverReplicateInContributionPhase and recoverReplicateInRevealPhase but we don't modify anything to this ChainReceipt. We better pass the blocknumber to those methods and create a new ChainReceipt(blockNumber, "") at the lowest level inside those two methods.

true yeah, I see your point

Ugo · 2019-03-18T14:01:59Z

src/main/java/com/iexec/core/replicate/ReplicateSupplyService.java

+            return RecoveryAction.REVEAL;
+        }
+
+        return null;


Let's return an optional rather than a null value. This is valid for all recoverReplicate* methods.

with pleasure

Ugo · 2019-03-18T14:07:54Z

src/main/java/com/iexec/core/replicate/ReplicateSupplyService.java

+            return RecoveryAction.ABORT_CONTRIBUTION_TIMEOUT;
+        }
+
+        if (task.getCurrentStatus().equals(TaskStatus.CONSENSUS_REACHED)) {


Those two ifs can be combined into one.

I thought it would be more readable, but it's okey let's change it

Ugo · 2019-03-18T14:12:32Z

src/main/java/com/iexec/core/replicate/ReplicateSupplyService.java

+        Optional<Task> oTask = taskService.getTaskByChainTaskId(replicate.getChainTaskId());
+        if (!oTask.isPresent()) return null;
+
+        return replicate.containsRevealedStatus()


Nice one but to difficult to read, can we simplify a bit? :)

of course, also done!

Ugo · 2019-03-18T14:25:55Z

src/main/java/com/iexec/core/task/TaskExecutorEngine.java


        executorMap.putIfAbsent(chainTaskId, ThreadPoolExecutorUtils.singleThreadExecutorWithFixedSizeQueue(1));

        Executor executor = executorMap.get(chainTaskId);
-        executor.execute(() -> taskService.tryUpgradeTaskStatus(chainTaskId));
+
+        return CompletableFuture.supplyAsync(() -> taskService.tryUpgradeTaskStatus(chainTaskId), executor)


Why do we need to return a completableFuture here? It seems that returning nothing is good enough no?

because in some cases I needed to wait for the execution to end.
Exp: ReplicateSupplyService > recoverReplicateInRevealPhase > L259

jeremyjams · 2019-03-18T14:54:09Z

src/main/java/com/iexec/core/replicate/Replicate.java

@@ -39,6 +40,27 @@ public ReplicateStatus getCurrentStatus() {
        return this.getLatestStatusChange().getStatus();
    }

+    @JsonIgnore
+    public ReplicateStatus getLastRelevantStatus() {


I think this method is very dangerous :/

But we need it to find out in what status the replicate was before trying to RECOVER (one or more times) and before being lost.
For instance: this is a typical exp: CONTRIBUTING > WORKER_LOST > RECOVERING > RECOVERING

jeremyjams

Bim

Ugo

Good for me. Thanks a lot!

zguesmi added 30 commits March 4, 2019 11:23

wip

ff76ff2

Merge branch 'master' into reconnect

d011cfe

Some refactoring

6372bc5

Send interrupted replicate list and handle recovery

891116c

use local iexec-common

a65ba8c

Merge branch 'master' into reconnect

7c6a6ee

Move logic from controller to service

ac9991f

update status according to replicate not status

218af3b

remove ambiguity

853d897

ignore workerLost and recovering statuses

20fe940

Deal with interrupted replicate in details

8c1ce12

update workflow to support recovering

973c6a3

cleaning

260aa84

Core does not need to know when a replicate is recovered

42ac767

Update tests

7c4dd95

Handle different cases for interrupted replicates

e66bc44

merge 'master' into reconnect branch

d51aaa3

Create replicateSupply service

9a0d9c9

Introduce replicate supply service

52dba96

Add missing actions to recovery

5a2a23b

Get tasks phase

2899de7

Fix tests

87f5771

Fix tests

aa9d488

Merge branch 'master' into reconnect

be233c2

Inject restTemplate

4e774e8

Remove REVEALED taskStatus

3cc6f89

Add tests to replicateSupplyService

a03221e

Replicate should complete task when necessary

8a0b7d0

Create restTemplate bean

4194e77

update branch

8c2819c

zguesmi and others added 5 commits March 15, 2019 18:08

Merge branch 'master' into reconnect

cd1887e

use remote common package

31523b5

Merge branch 'reconnect' of github.com:iExecBlockchainComputing/iexec…

d48d29b

…-core into reconnect

Refactoring

527f301

Fix tests

5c84f5d

Ugo requested review from Ugo and jeremyjams March 18, 2019 08:38

Ugo suggested changes Mar 18, 2019

View reviewed changes

jeremyjams reviewed Mar 18, 2019

View reviewed changes

zguesmi added 3 commits March 18, 2019 17:27

Add some requested changes

df43eeb

Pass blockNumber instead of chainReceipt

6ed80a0

Merge branch 'master' into reconnect

5838c3b

jeremyjams approved these changes Mar 19, 2019

View reviewed changes

Ugo approved these changes Mar 19, 2019

View reviewed changes

zguesmi merged commit c6c16e2 into master Mar 19, 2019

zguesmi deleted the reconnect branch March 19, 2019 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle disconnection/reconnection of worker and help it recover interrupted replicates #234

Handle disconnection/reconnection of worker and help it recover interrupted replicates #234

zguesmi commented Mar 15, 2019 •

edited

Loading

Ugo left a comment

Ugo Mar 18, 2019

zguesmi Mar 18, 2019 •

edited

Loading

Ugo Mar 18, 2019

zguesmi Mar 18, 2019

Ugo Mar 18, 2019

zguesmi Mar 18, 2019

Ugo Mar 18, 2019 •

edited

Loading

zguesmi Mar 18, 2019

Ugo Mar 19, 2019

zguesmi Mar 19, 2019

Ugo Mar 18, 2019

zguesmi Mar 18, 2019

Ugo Mar 18, 2019

zguesmi Mar 18, 2019

Ugo Mar 18, 2019

zguesmi Mar 18, 2019

Ugo Mar 18, 2019

zguesmi Mar 18, 2019

jeremyjams Mar 18, 2019

zguesmi Mar 18, 2019

jeremyjams left a comment

Ugo left a comment

Handle disconnection/reconnection of worker and help it recover interrupted replicates #234

Handle disconnection/reconnection of worker and help it recover interrupted replicates #234

Conversation

zguesmi commented Mar 15, 2019 • edited Loading

Ugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zguesmi Mar 18, 2019 • edited Loading

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

Ugo Mar 18, 2019 • edited Loading

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

jeremyjams left a comment

Choose a reason for hiding this comment

Ugo left a comment

Choose a reason for hiding this comment

zguesmi commented Mar 15, 2019 •

edited

Loading

zguesmi Mar 18, 2019 •

edited

Loading

Ugo Mar 18, 2019 •

edited

Loading