Skip to content

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
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
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.

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