-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add ContributeAndFinalized
replicate statuses
#408
Add ContributeAndFinalized
replicate statuses
#408
Conversation
@@ -132,6 +158,8 @@ public static List<ReplicateStatus> getCompletableStatuses() { | |||
RESULT_UPLOADING, | |||
RESULT_UPLOAD_FAILED, | |||
RESULT_UPLOADED, | |||
CONTRIBUTE_AND_FINALIZE_ONGOING, |
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.
Are we sure as REVEALING
is not part of this list ?
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.
As far as I understand this part, this list should return all "non-uncompletable statuses", which is not the case right now. Also, it is used only for tests in iexec-core.
Let's rework the tests in iexec-core so that we can remove this method.
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.
Honestly, I would go for the consistency in a first step and to go for the cleanup in a second one.
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.
Are you sure about your "non-uncompletable" ?
For me it is the list of states allowing to go to COMPLETING
state which are
REVEALED
(worker part of the consensus)CONTRIBUTE_AND_FINALIZE_DONE
(worker has revealed and has uploaded its results)RESULT_UPLOAD
something as the worker is part of the consensus and if this worker fails, to upload, another one could do it (to check if it is really supported)
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.
Please have a look at the only use of this method to understand what I mean:
https://github.com/iExecBlockchainComputing/iexec-core/blob/1e6d93d197b0346c4b46a62278d5b7286e56dd0f/src/test/java/com/iexec/core/replicate/ReplicateListenersTests.java#L164-L175
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.
After some discussions, we've decided to remove CONTRIBUTE_AND_FINALIZE_ONGOING
from this list.
Please see b585b98.
@@ -160,6 +168,9 @@ public static List<ReplicateStatus> getFailableStatuses() { | |||
CONTRIBUTED, | |||
REVEALING, | |||
REVEAL_FAILED, | |||
CONTRIBUTE_AND_FINALIZE_ONGOING, | |||
CONTRIBUTE_AND_FINALIZE_DONE, |
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 am not sure about keeping CONTRIBUTE_AND_FINALIZE_DONE
here as the REVEALED
state is not present in this list.
When CONTRIBUTE_AND_FINALIZE_DONE
, result has been CONTRIBUTED
and PROVED
(aka REVEALED
) on chain. I am not sure the task can be failed in this case.
I would rather comment this and ask a question to our experts when they are back.
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 do agree with your argument. Thanks for pointing it out!
Please see 258f8ae.
@@ -132,6 +158,8 @@ public static List<ReplicateStatus> getCompletableStatuses() { | |||
RESULT_UPLOADING, | |||
RESULT_UPLOAD_FAILED, | |||
RESULT_UPLOADED, | |||
CONTRIBUTE_AND_FINALIZE_ONGOING, |
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.
Honestly, I would go for the consistency in a first step and to go for the cleanup in a second one.
Kudos, SonarCloud Quality Gate passed! |
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.
Looks good to me !
LGTM !
No description provided.