Skip to content

Conversation

@zguesmi
Copy link
Member

@zguesmi zguesmi commented May 22, 2019

No description provided.

switch (interruptedReplicate.getRecoveryAction()) {
case WAIT:
subscriptionService.subscribeToTopic(chainTaskId);
resultService.saveResultInfo(chainTaskId, replicateModel);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, couldn't we resultService.saveResultInfo(chainTaskId, replicateModel); once for all at the beginning of the recoverReplicate(..) method (so we don't miss any case)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't need it for all cases.

case COMPLETE:
taskExecutorService.completeTask(chainTaskId);
break;
if (!isResultAvailable && recoveryAction != RecoveryAction.CONTRIBUTE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about something like:

isResultRequired = (recoveryAction == WAIT ||recoveryAction == CONTRIBUTE || recoveryAction == REVEAL recoveryAction == UPLOAD_RESULT);
if(!isResultAvailable && isResultRequired){
  log.error(..)
  continue;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's clearer. But when recoveryAction == CONTRIBUTE the result is not necessarily needed because we can re-do the computation.
So I'd say:
isResultRequired = Arrays.asList(WAIT, REVEAL, UPLOAD_RESULT).contains(recoveryAction);

Copy link

@Ugo Ugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems all good for me Zied! Thank you

@zguesmi zguesmi merged commit de4b827 into master May 27, 2019
@zguesmi zguesmi deleted the patch branch May 27, 2019 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants