Skip to content
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

Merged
merged 10 commits into from
May 3, 2023

Conversation

mcornaton
Copy link
Contributor

No description provided.

@mcornaton mcornaton requested a review from jbern0rd April 25, 2023 09:05
@mcornaton mcornaton marked this pull request as ready for review April 25, 2023 09:15
@@ -132,6 +158,8 @@ public static List<ReplicateStatus> getCompletableStatuses() {
RESULT_UPLOADING,
RESULT_UPLOAD_FAILED,
RESULT_UPLOADED,
CONTRIBUTE_AND_FINALIZE_ONGOING,
Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

@mcornaton mcornaton requested a review from jbern0rd April 28, 2023 07:27
@@ -160,6 +168,9 @@ public static List<ReplicateStatus> getFailableStatuses() {
CONTRIBUTED,
REVEALING,
REVEAL_FAILED,
CONTRIBUTE_AND_FINALIZE_ONGOING,
CONTRIBUTE_AND_FINALIZE_DONE,
Copy link
Contributor

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.

Copy link
Contributor Author

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,
Copy link
Contributor

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.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@jbern0rd jbern0rd left a 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 !

@mcornaton mcornaton merged commit 33d68b8 into develop May 3, 2023
@mcornaton mcornaton deleted the feature/contribute-and-finalize-replicate-status branch May 3, 2023 07:05
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.

2 participants