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

Minor cleanups and clarifications #303

Merged
merged 4 commits into from
Oct 29, 2020
Merged

Minor cleanups and clarifications #303

merged 4 commits into from
Oct 29, 2020

Conversation

rhc54
Copy link
Member

@rhc54 rhc54 commented Oct 28, 2020

Add reserved prefixes for regular expressions and explain that such
prefixes are required to be present. Expand on explanation of refresh
cache behavior and required keys. Add new compressed byte object data
type.

Signed-off-by: Ralph Castain rhc@pmix.org

Add reserved prefixes for regular expressions and explain that such
prefixes are required to be present. Expand on explanation of refresh
cache behavior and required keys. Add new compressed byte object data
type.

Signed-off-by: Ralph Castain <rhc@pmix.org>
@rhc54 rhc54 added this to the PMIx v4 Standard milestone Oct 28, 2020
@rhc54 rhc54 requested a review from jjhursey October 28, 2020 20:55
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.

A note on the placement of the new regex prefixes WRT the old prefixes that needs to be resolved. Otherwise, the rest of the PR looks fine to me.

\item \emph{compress} - a compressed format based on the zlib compression package
\end{itemize}

Note that the regular expression itself is not required to be a printable string nor to obey typical string constraints (e.g., include a \code{NULL} terminator byte).
Copy link
Member

Choose a reason for hiding this comment

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

We need to update the "Advice to PMIx server hosts" in both the PMIx_generate_regex and PMIx_generate_ppn functions (16.2.1 and 16.2.2). That advice is where we introduce the colon delimiter with either the pmix:\0 prefix or the blob:\0 prefix. Note that we added the :\0 (with the NUL character) so that if the user tired to print it they wouldn't accidentally be printing an unterminated string. I think that it would be good to preserve that.

From this addition it seems like native: == pmix: and compress: == blob:. Why do we need the name change?

I think this text should be moved to PMIx_generate_regex (with a reference to it in PMIx_generate_ppn) as normal text since it is putting a requirement on those two functions. Then trim up the Advice blocks since some of that is normative text now.

In this particular section, it would be good to keep a sentence or two about the regex nature of the returned "string" specifically indicating that the return value is not necessarily printable (maybe keep your last sentence here).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sigh - just shows you shouldn't try to write documents when half asleep and multi-tasking on a telecon 😄 I just knew I had written the prefix stuff up somewhere, but couldn't find it!

I'll do as suggested and push the change a little later - after I have completely woken up this time!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jjhursey Okay, see what you think now!

Per the provided comments

Signed-off-by: Ralph Castain <rhc@pmix.org>
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.

This is looking good. I had 1 minor item, and 1 suggested change for discussion.

rhc54 and others added 2 commits October 29, 2020 12:23
Looks good to me.

Signed-off-by: Ralph Castain <rhc@pmix.org>

Co-authored-by: Josh Hursey <4259120+jjhursey@users.noreply.github.com>
Signed-off-by: Ralph Castain <rhc@pmix.org>

Co-authored-by: Josh Hursey <4259120+jjhursey@users.noreply.github.com>
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.

👍

@rhc54 rhc54 merged commit fa79f74 into pmix:v4 Oct 29, 2020
@rhc54 rhc54 deleted the rfc/v4fix branch October 29, 2020 20:57
@jjhursey jjhursey mentioned this pull request Oct 30, 2020
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