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

Implement regx/zlib component #6936

Closed
sam6258 opened this issue Aug 27, 2019 · 6 comments
Closed

Implement regx/zlib component #6936

sam6258 opened this issue Aug 27, 2019 · 6 comments

Comments

@sam6258
Copy link
Contributor

sam6258 commented Aug 27, 2019

Create a new, non-default regx component in the v4.0.x and v3.1.x branches that compresses the node/vpid string using zlib and communicates the compressed string all of the time (doesn't send over command line).

Motivation for this component is a functional path where fwd, reverse regx components fail. See discussion on #6915 for more info.

@sam6258
Copy link
Contributor Author

sam6258 commented Aug 27, 2019

One problem that may cause us to revisit if we want to implement this:

The orte_node_regex by default is a string. Many of the lengths used for the communication of the regex are based on the null terminator (strlen). I think implementing the zlib regx component would require us to change the prototypes for the nidmap_create and nidmap_parse functions because the regex string will stop at the first null byte in the compression data. We would need to change the char **regex and char *regex arguments of those functions to opal_buffer_t. Doing this would require us to break compatibility for forked repositories (if someone has a custom regx component on v4.0.x branch, they will have to refactor when they pull in the new v4.0.x).

This seems like it would be a larger change than we initially intended. Thoughts here?

@jjhursey
Copy link
Member

@gpaulsen @hppritcha be advised that we have stopped work on this zlib component until we receive guidance on whether the change will be "too big" for v4.0.x. We don't want to invest work in something that the RM's will reject when it's done for that reason.

The scope of changes are bigger than just a new component - we will have to, at least, change internal interfaces to the regx framework, all of the regx components, and the protocol by which the nidmap regex is transmitted to the daemons on startup (to pass size information).

Note on master this change already happened to change the nidmap from a char ** to an opal_buffer_t. We would try to match that technique customized for the v4.0.x branch.

@gpaulsen
Copy link
Member

I'm not sure we'd want to change a framework API on a release branch.
If that's true, and since this isn't needed on master, I think the only other possibility would be to somehow engineer this new proposed component in a way that it can keep with the existing regex framework on v3.x and v4.x.
That might be possible if we use zlib to compress the data, and then use an encoding / decoding phase to wrap the compressed data. But at that point, it seems like this might be to risky to add to a release branch.
Lets keep this issue open in case someone else can come up with a good design.

@hppritcha
Copy link
Member

We don't plan to implement this in the v4.0.x and older series.

@jsquyres
Copy link
Member

jsquyres commented Apr 6, 2021

Would this be a PRTE issue now (and not an Open MPI issue)?

@jjhursey
Copy link
Member

jjhursey commented Apr 7, 2021

AFAIK PRRTE already uses zlib (where available) to compress this information. If zlib is not available then it is sent in an uncompressed form. There is no custom regex parser any longer.

@jjhursey jjhursey closed this as completed Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants