-
Notifications
You must be signed in to change notification settings - Fork 10
Notification type for recovery #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| ContributionAuthorization contributionAuth = interruptedReplicate.getContributionAuthorization(); | ||
| RecoveryAction recoveryAction = interruptedReplicate.getRecoveryAction(); | ||
| ContributionAuthorization contributionAuth = missedTaskNotification.getTaskNotificationExtra().getContributionAuthorization(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contributionAuth is not going to be there every time, instead, we can get the chainTaskId from the taskNotification itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
|
||
| if (!oReplicateModel.isPresent()) { | ||
| if (!optionalTaskDescription.isPresent()) { | ||
| log.error("Could not recover task, no replicateModel retrieved [chainTaskId:{}, RecoveryAction:{}]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe replace "replicateModel" with "task description" for consistency sake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| @PostConstruct | ||
| public void initIt() { | ||
| corePublicAddress = customFeignClient.getPublicConfiguration().getSchedulerPublicAddress(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use "publicConfigurationService.getSchedulerPublicAddress()" instead of sending another HTTP request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @Async | ||
| private String compute(AvailableReplicateModel replicateModel) { | ||
| ContributionAuthorization contributionAuth = replicateModel.getContributionAuthorization(); | ||
| private String compute(ContributionAuthorization contributionAuth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the goal is to use "contributionAuth" only in the contribute() logic we should pass the TaskDescription instead. That would also remove the repeated call "getTaskDescriptionFromChain(chainTaskId)".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the initial plan, but in a future PR :)
| } | ||
|
|
||
|
|
||
| public void tryToContribute(ContributionAuthorization contributionAuth) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to replace this method by an "isReadyToContribute()" method and use it in the subscriptionService so the latter stays the entrypoint that explicitly creates replicates (even when recovering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we are going to tune that in a second time
No description provided.