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

is this the desired output for flux-up? #855

Closed
chu11 opened this issue Oct 18, 2016 · 17 comments
Closed

is this the desired output for flux-up? #855

chu11 opened this issue Oct 18, 2016 · 17 comments

Comments

@chu11
Copy link
Member

chu11 commented Oct 18, 2016

Just messing around and noticed

$ flux up --newline 
ok:     
[0
1
2
3]
slow:   
fail:   
unknown:

Is this what is actually desired? whatsup and nodeattr don't have the brackets b/c presumably the user wants comma/newline output for easy parsing.

As an aside, also noticed no tests for flux-up either.

@garlick
Copy link
Member

garlick commented Oct 18, 2016

That's probably a mistake, and some test coverage would be welcome, though t/t0010-basic-utils.t does call it.

You may have noticed this command doesn't really do much when the "live" module isn't loaded, which now it isn't by default. That doesn't mean some tests wouldn't be good to have though.

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

I'll take a crack at this. Didn't notice it was in the testsuite under generic-utils. I can add some newline or comma equivalent tests.

@garlick
Copy link
Member

garlick commented Oct 18, 2016

That would be great!

@grondo
Copy link
Contributor

grondo commented Oct 18, 2016

IMO, --newline or --comma don't make much sense outside of -u, --up or -u, --down, though I guess it doesn't hurt to allow it. It would also be nice if --newline and --comma were combined into a --delim=STRING argument for more flexibility, but again that is just my opinion.

@garlick
Copy link
Member

garlick commented Oct 18, 2016

That works for me. In a bootstrap instance, this could replace whatsup so think of it that way?

@grondo
Copy link
Contributor

grondo commented Oct 18, 2016

Yes, though eventually it might need a way to map rank to hostname

@garlick
Copy link
Member

garlick commented Oct 18, 2016

Excellent point. Too bad hwloc doesn't grab that and drop it in the kvs under resource.hwloc.by_rank

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

IIRC, a long time ago the plan was flux could eventually replace cerebro and maybe even other monitoring subsystems. whatsup would just "point" to flux to get the equivalent up/down information.

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

ugh, because of the labels (e.g. slow:) there's some special case handling for newlines if we don't limit --delim=STRING to just --up and --down. I'm inclined to leave --comma and --newline as is, as its the style I think we're familiar with.

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

Nevermind. I originally took the --delim=STRING literally, as in a random string could be the separator. But if it's a char, it's no biggie. We'll go with that.

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

I ended up settling on keeping --newline and --comma as the first time I had to do --delim=$'\n', I thought perhaps this isn't the best option for most end users.

@grondo
Copy link
Contributor

grondo commented Oct 18, 2016

Interesting why was the $ required?

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

The shell won't compile "\n" into a newline character automatically. $'' is the Bash way to do it make it happen.

@grondo
Copy link
Contributor

grondo commented Oct 18, 2016

weird! I always use it like:

 $ hostlist -e -d '\n' foo[1-4]                                                                                                                    
foo1
foo2
foo3
foo4

Learned something new!

@chu11
Copy link
Member Author

chu11 commented Oct 18, 2016

Ahh, you're specifically handling it as a special case in hostlist

function convert_string_to_escape (s)
        local t = { t='\t', n='\n', f='\f', v='\v', t='\t', r='\r',}
        return s:sub(1,1) == '\\' and t[s:sub(2,2)] or s
end

I was about to do something like that, but it felt hackish. Do you really have your heart set on --delim? :-)

@grondo
Copy link
Contributor

grondo commented Oct 18, 2016

No I just had no idea about that limitation! What you are doing is fine with me!

@chu11
Copy link
Member Author

chu11 commented Oct 19, 2016

Fixed via #858

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

No branches or pull requests

3 participants