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

rfc24: Add redirect event #206

Merged
merged 1 commit into from Sep 27, 2019
Merged

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Sep 27, 2019

(string) The stream name (e.g. `stdout`, `stderr`). All valid stream
names MUST appear as keys in the header `encoding` object.

The following keys are OPTIONAL in the event context object:
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought, we may want to add an optional rank key to indicate the tasks to which the redirect applies. This would allow some tasks to be redirected and not others in the future. (e.g. redirect all tasks except rank 0 to /dev/null)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I went back and forth on that. Now that I think about it, I suppose if the redirect applies to all ranks, the rank be == all? So we should make the rank a requirement.

Copy link
Member Author

@chu11 chu11 Sep 27, 2019

Choose a reason for hiding this comment

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

oh wait, ranks are idset strings. Hmmm. Should we allow idset string or "all"? Just trying to think of how shell & job-attach handle this. one line per task is cumbersome for the "all" case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be too difficult to generate the correct idset in the shell output plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, that shouldn't be too hard.

I was mostly thinking about how to recognize that all tasks have been redirected, but we can can count idsets just like EOFs and we'd be fine.

I was also hoping to output a single message if both stdout/stderr were redirected to the same file, but I suppose that can be two messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also hoping to output a single message if both stdout/stderr were redirected to the same file, but I suppose that can be two messages.

Ah, that is a good point. Maybe stream could be a list? Eh, maybe two events is ok like you say.

@chu11
Copy link
Member Author

chu11 commented Sep 27, 2019

re-pushed adding ranks

@chu11
Copy link
Member Author

chu11 commented Sep 27, 2019

re-pushed with different filename in example, to pass spellcheck

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

@grondo
Copy link
Contributor

grondo commented Sep 27, 2019

Ok, merging.

@grondo grondo merged commit 0bd55b9 into flux-framework:master Sep 27, 2019
@chu11 chu11 deleted the issue2399rfc branch August 12, 2022 19: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.

None yet

3 participants