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.

Sign up for GitHub

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

Jump to bottom

Return codes #350

Merged
merged 9 commits into from
Nov 22, 2021
Merged

Return codes #350

merged 9 commits into from
Nov 22, 2021

Conversation

dsolt
Copy link
Contributor

@dsolt dsolt commented May 17, 2021

A review of how return values are presented for each API to be more consistent.

@dsolt
Copy link
Contributor Author

dsolt commented May 17, 2021

Maybe take a look at these files (based on the order listed in the "files" section of review)

@kathrynmohror - 1-4 files
@spophale - 5-9 files
@jjhursey - 10-13
@dsolt - 14-15

rhc54
rhc54 reviewed May 18, 2021
View reviewed changes
jjhursey
jjhursey reviewed May 24, 2021
View reviewed changes
jjhursey
jjhursey reviewed May 24, 2021
View reviewed changes
jjhursey
jjhursey reviewed May 24, 2021
View reviewed changes
jjhursey
jjhursey reviewed May 24, 2021
View reviewed changes
Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

Finished review of files 10-13

@dsolt
Copy link
Contributor Author

dsolt commented Jun 22, 2021

pmix-standard.pdf

@jjhursey jjhursey added Eligible Eligible for consideration by ASC and removed WorkInProgress Work In Progress labels Jun 28, 2021
@jjhursey
Copy link
Member

Please use emoji reactions ON THIS COMMENT to indicate your position on this proposal.

  • You do not need to vote on every proposal
  • If you have no opinion, don't vote - that is also useful data
  • If you've already commented on this issue, please still vote so
    we know your current thoughts
  • Not all proposals solve exactly the same problem, so we may end
    up accepting proposals that appear to have some overlap
    This is not a binding majority-rule vote, but it will be a very
    significant input into the corresponding ASC decision.

Here are the meanings for the emojis:

  • Hooray or Rocket: I support this so strongly that I
    want to be an advocate for it
  • Heart: I think this is an ideal solution
  • Thumbs up: I'd be happy with this solution
  • Confused: I'd rather we not do this, but I can tolerate it
  • Thumbs down: I'd be actively unhappy, and may even consider
    other technologies instead
    If you want to explain in more detail, feel free to add another
    comment, but please also vote on this comment.

jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey reviewed Jul 20, 2021
View reviewed changes
jjhursey
jjhursey requested changes Jul 22, 2021
View reviewed changes
Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

I was verifying that the revision exceptions were applied. They all are except the one noted here. Please make that change and then it will be ready for second vote in the next quarterly meeting.

@jjhursey
Copy link
Member

PMIx ASC 3Q 2021

  • Passed the first vote with revision exception: 8 yes / 0 no / 1 abstain
    • Failed the first vote without revision exception: 2 yes / 6 no / 1 abstain
    • Revision exception contents required have been committed.
    • @dsolt Verification turned up one that was missed in the last commit that needs to be addressed.
  • Will be eligible for second vote in 4Q 2021

@jjhursey jjhursey added the First Vote Passed ASC first vote passed label Jul 26, 2021
@jjhursey jjhursey added this to the PMIx v5 Standard milestone Oct 28, 2021
@jjhursey
Copy link
Member

jjhursey commented Nov 3, 2021

@dsolt See the comment above about a Revision Exception change that needs to be resolved before we can merge.

PMIx ASC 4Q 2021

  • Passed the second vote with revision exception: 13 yes / 0 no / 0 abstain

@jjhursey jjhursey added Accepted as Stable ASC second vote passed. Accepted as Stable! Major Text Change labels Nov 3, 2021
@jjhursey
Copy link
Member

The signed-off checker stumbled on the "Co-authored-by" - so it is fine to merge.

dsolt and others added 9 commits November 22, 2021 13:39
@dsolt @jjhursey
Initial commit for return code macro changes
554336d 
Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Update chapter on synchronization to use new approach to return codes
a9dfa18 
Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Fix unrelated typo
2914c54 
Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Feedback from WG meeting
d4f05d5 
Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Remaining chapters
28e03bd 
Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Feedback from reviewers
eadbbe9 
Signed-of-by:  dsolt@us.ibm.com

Mostly cases of using constdesc instead of itemize in lists.
Cases of not using the macros when I should have.
Cases of adding non-blocking PMIX_OPERATION_SUCCEEDED where I thought it was missing,
but in reality those API's can't return that value
A few other misc fixes.
@dsolt @jjhursey
Add better description for PMIX_ERR_INIT
03c91bc 
Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Feedback from first reading.
c68ed90 
Possible revision exceptions:
* Get rid of redundant text around cbfunc not being called on error
* 2 cases where returnsimple was used where returnsimplenb was intended
* Get rid of PMIX_ERR_BAD_PARAM when it adds no useful information to the API
* Remove claim that PMIX_OPERATION_SUCCEEDED can only be called when cbfunc
is non-null for API's where it is permitted to called for a null cbfunc.
* Clarify in returnsimplenb macro that the cbfunc is only called when
PMIX_SUCCESS is returned.
* Remove implementation assumption from nonblocking macro that assumes processing
is done by the "host environment"

Signed-of-by:  dsolt@us.ibm.com
@dsolt @jjhursey
Fix PMIX_OPERATION_SUCCEEDED description for PMIx_Deregister_event_ha…
d46aa8a 
…ndler

Co-authored-by: Josh Hursey <4259120+jjhursey@users.noreply.github.com>
Signed-off-by: Joshua Hursey <jhursey@us.ibm.com>
@jjhursey
Copy link
Member

I just rebased the branch so it will merge cleanly. Ready to merge once CI is done.

@jjhursey jjhursey merged commit dd1cc36 into pmix:master Nov 22, 2021
@jjhursey jjhursey mentioned this pull request Mar 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhc54 rhc54 rhc54 left review comments

@jjhursey jjhursey jjhursey approved these changes

Assignees
No one assigned
Labels
Accepted as Stable ASC second vote passed. Accepted as Stable! Clarification Eligible Eligible for consideration by ASC First Vote Passed ASC first vote passed Major Text Change
Projects
None yet
Milestone
PMIx v5 Standard
Development

Successfully merging this pull request may close these issues.
3 participants
@dsolt @jjhursey @rhc54