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

flux-kvs needs cleanup #897

Closed
garlick opened this Issue Nov 4, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@garlick
Copy link
Member

garlick commented Nov 4, 2016

As noted in #896, flux-kvs option handling is a bit of a mess. In fact, the utility accumulated options and subcommands mainly to support tests that probably should have been supported by a purpose-built program in t/kvs.

At minimum it should be reworked to use optparse to handle its options.

It could also be converted into a a "builtin" command.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 4, 2016

As another cleanup topic, the count to watch-dir initially did not do what I thought it would. I thought count would sit and and wait for N changes to a key to happen. But instead, it outputs the current value and waits for "N-1" changes to happen. So count is more "output N key values", not "output N changes to key values".

I just replicated this behavior in watch for #896. But perhaps an option for both behaviors could be added in a cleanup.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 4, 2016

So count is more "output N key values", not "output N changes to key values".

I'd possibly consider this a bug, or at least surprising behavior, i.e. it implies the "default" value for watch count is 2? However, I would hope the interface will end up being fully cleaned up in resolution of this issue.

It would also be really nice if watch and watch-dir were combined (in the API too), but that may just be wishful thinking.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 4, 2016

@grondo The default count is infinity, so it's just watching forever. Ya know, I didn't even think about this being a bug, partially because it wasn't documented so I just assumed the current behavior was what was intended.

Hmmm, watch and watch-dir by default output the current value of the key before "watching" anything. Maybe it shouldn't? Does watch imply "only show me changes"? I'm somewhat leaning to the belief it shouldn't show the current value. So count behavior would change as a result.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 4, 2016

That's what was intended and the count was added later to support tests as
I recall.

One thought is to just wholesale move the existing tool to the test dir and
start from scratch on a user facing command. Testing motivated a lot of
the convoluted evolution of the old tool so I think the new thing could be
trimmed down a lot and could maybe line up less with the API which is
planned to change and more with usability.

On Nov 4, 2016 9:38 AM, "Al Chu" notifications@github.com wrote:

@grondo https://github.com/grondo The default count is infinity, so
it's just watching forever. Ya know, I didn't even think about this being a
bug, partially because it wasn't documented so I just assumed the current
behavior was what was intended.

Hmmm, watch and watch-dir by default output the current value. Maybe it
shouldn't? watch might imply "only show me changes"?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#897 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKX20-Haqb8Ew4KQN2dQMTEyJaJc4_Nks5q6193gaJpZM4KprLR
.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 4, 2016

@grondo The default count is infinity, so it's just watching forever.

Oh yeah, duh, I meant to say "to wait for a single change the count is 2"

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Nov 4, 2016

@garlick Skimming the libkvs header file, I can see what you mean now that flux-kvs appears to follow a lot of the libkvs API. If we move it into the test dir, we can probably trim it down a nice chunk.. My newly added -d options, which are predominantly for user convenience on the command line, could be axed.

@chu11 chu11 self-assigned this Nov 8, 2016

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Dec 8, 2016

@garlick for the cleaned up flux-kvs tool, do you have an opinion on keeping or removing the "treeobj" commands? I can see why they were initially added to test the "treeobj" commands, but I don't see the use case a user would have for them.

Or perhaps it makes more sense for a "treeobj" option to be available to the "get" and similar subcommands.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 8, 2016

Probably ok to leave those out of the user-facing program, at least until somebody thinks of a use.

@chu11 chu11 referenced this issue Dec 9, 2016

Merged

minor KVS cleanup #919

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Dec 14, 2016

Was debating what other commands in flux-kvs could be axed. I axed type b/c it seemed to be for testing kvs JSON internals, dirsize b/c shell scripters are more likely to pipe to wc -l anyways, and exists b/c shell scripters will just check != empty string on a get.

I'm a little more borderline on link, readlink, and wait. Do we anticipate users wanting to use these?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 14, 2016

I can't think of reason not to keep link and readlink, but I may not understand the purpose of the flux-kvs replacement yet.

The dirsize and exists subcmds are also nice because you can check existence of keys and number of entries in kvs dir without actually fetching all the values, which could be a heavyweight operation for large values and/or dirs. However, I don't have specific use cases for flux-kvs except casual cmdline use, and in that case flux kvs {put, get, dir} would possibly be sufficient for now...

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Dec 14, 2016

@grondo Yeah, that was just my thinking. If it's just for casual cmdline use, then it seems excessive to have a command mapping to each potential kvs lib function.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Dec 28, 2016

I believe this should be closed now that #928 is merged.

@garlick garlick closed this Dec 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.