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

initial flux-kvs cleanup #928

Merged
merged 27 commits into from Dec 22, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Dec 15, 2016

Per #897 , cleaned up a bunch of flux-kvs command, trying to make it a more "user facing" tool instead of the unit-testing tool it was before. Refactor w/ liboptparse along the way. Copied the old tool into t/kvs/basic for unit testing.

Ended up axeing get-treeobj, put-treeobj, getat, dirat, readlinkat, type, dirsize, and exists as I felt they were more for testing the KVS api & implementation than really something users need. Casual users will just use wc -l or -n $str in scripts with dir and get.

Other minor fixes along the way:

  • make watch command logic more in line with user thinking that it means "N changes" occured. Add -o option to output current value.
  • Rename -r to -R for consistency to ls
  • merge dropcache and dropcache-all into one command
  • count turned into an option

There's still a few more things that I think could be cleaned up, but they will probably be for a future PR.

  • dropcache --all, could be a KVS lib function or an option in kvs_dropcache() or perhaps could be per rank if tweaked appropriately. Sending an event message doesn't seen necessary.
  • merging watch and watch-dir into one. Maybe can be done right now, but not enough details in kvs.h about what functions can return EISDIR to program at the moment. Need to dig into libkvs deeper.
  • various lingering FIXMEs in flux-kvs and kvs

The help output from flux-kvs via liboptparse is IMO a little ugly

Usage: flux-kvs copy source destination
   or: flux-kvs copy-fromkvs key file
   or: flux-kvs copy-tokvs key file
   or: flux-kvs dir [-R] [-d] [key]
   or: flux-kvs dropcache [--all]
   or: flux-kvs get key [key...]
   or: flux-kvs link target linkname
   or: flux-kvs mkdir key [key...]
   or: flux-kvs move source destination
   or: flux-kvs put key=value [key=value...]
   or: flux-kvs readlink key [key...]
   or: flux-kvs unlink key [key...]
   or: flux-kvs version 
   or: flux-kvs wait version
   or: flux-kvs watch [-o] [-c count] key
   or: flux-kvs watch-dir [-R] [-d] [-o] [-c count] key
  -h, --help             Display this message.

I think some tweeks to the liboptparse library could make this nicer (and possibly could be taken advantage by flux.c). Perhaps it could be something more like:

Usage: flux-kvs [OPTIONS] <command> [<args>]
  -h, --help             Display this message.

flux-kvs commands
  copy           do something
  copy-fromkvs   do something else
  ... 

Still need to add basic tests for the new command (edit: and update old tests that used flux kvs), so don't hit merge yet. Or perhaps I should work on the liboptparse help output first.

@garlick garlick added the review label Dec 15, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 15, 2016

Coverage Status

Coverage decreased (-0.6%) to 75.717% when pulling d460d7a on chu11:kvscleanup2 into 82e5275 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 15, 2016

The help output from flux-kvs via liboptparse is IMO a little ugly

That is just the default help output, you can override that as we do in flux command. I think the huge number of subcommands is not the common case and I'd probably argue against optimizing for that in liboptparse directly. (though it is true that the subcommand help output is pretty bare bones.)

It might also be really nice if commands were listed in order of usefulness as well instead of alphabetical order, e.g. flux-kvs get and flux-kvs put should possibly come first ? I'm not sure what others might think about that.

I think some tweeks to the liboptparse library could make this nicer (and possibly could be taken advantage by flux.c).

The flux help output is already optimized via autogenerated help strings for both builtin and external commands, so I'm not sure there is anything better liboptparse could do there.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2016

Ahhh, I see what you're talking about now. Actually, looking into liboptparse a bit more, overriding it for flux-kvs is probably the best case here.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 15, 2016

Could the copy commands be squashed into one subcommand? (Possibly itself with subcommands?)

Also, watch and watch-dir would seem to be candidates for becoming a single command.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2016

@grondo Yup, watch and watch-dir is something I'm already looking at. I got a little stuck the last time I tried to merge it.

Hadn't thought about the copy commands. Are you thinking something like copy [--srcfile X] [--destfile X] key?

Hmmm, now that I"m thinking about it, do the copy-fromkvs and copy-tokvs commands really need to be in the basic tests? I"m thinking maybe they don't now.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 15, 2016

Hadn't thought about the copy commands. Are you thinking something like copy [--srcfile X] [--destfile X] key?

Well I hadn't looked at the actual commands in awhile, but I was thinking something akin to cpio, but that command has a horrible interface IMO, so perhaps a bad example. I forgot that flux kvs copy was actually to copy value from one key to another, so I don't think it goes with the copy-* commands anymore. At least copy-tokvs and copy-fromkvs could be combined into a cpio (copy-in-out) command.

Also, I wonder how others feel about using flux-kvs cp and flux-kvs mv for copy and move since they are equivalent to filesystem counterparts?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2016

I was thinking of renaming a few things like copy to cp, but I go back and forth on it. There are certainly "filesystem-isms" amongst the tool, but at the same time there are things that are "database-y". Like would I rename unlink to rm? Or would del be better?

Hmmm, how about something as simple as cpio <-i | -o> <file> <key> to merge the two copies?

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch from d460d7a to 4e6530c Dec 20, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 20, 2016

Current coverage is 76.08% (diff: 78.68%)

Merging #928 into master will increase coverage by 0.05%

@@             master       #928   diff @@
==========================================
  Files           149        149          
  Lines         26013      25936    -77   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19778      19734    -44   
+ Misses         6235       6202    -33   
  Partials          0          0          
Diff Coverage File Path
••••••• 78% src/cmd/flux-kvs.c

Powered by Codecov. Last update a1fe845...0702878

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 20, 2016

This is my next round of flux-kvs cleanup, this should approach the final cleanup as many unit tests have been added.

Biggest cleanup since the last push is merging watch and watch-dir into a single command. It handles many corner cases compared to the original, such as noticing/detecting between keys and dirs, noticing if keys/dirs have been removed, and noticing if keys have been converted to dirs and vice versa. The function cmd_watch is a little long-winded, but hopefully it makes sense.

unlink also has semantics more akin to rm, with -R necessary for recursive deletes and errors if a entry doesn't exist.

cpio is the merger of copy-tokvs and copy-fromkvs. Maybe the interface isn't the best, PLMK if something is obviously better.

I went back and forth about creating a kvs_dropcache_all() function, but eventually didn't, as I couldn't decide on the usefulness and/or if the kvs_dropcache() call really needed to take a rank variable. I figure if such cleanup is warranted, that can be done in the future.

chu11 added some commits Dec 1, 2016

test/basic: Add new program for kvs testing
To prepare for refactoring/cleanup of flux-kvs, copy the current
flux-kvs command into new kvs "basic" tool, for testing basic
functionality of the kvs.  Rename tool to 'basic' accordingly.
test/kvs: Adjust kvs basic tests
Adjust basic KVS tests to use kvs/basic internally build test tool.
test/kvs: Remove kvs dir -d tests
The -d option was for user output convenience and not necessary
for basic kvs tests.
test/kvs: Remove multi-input in basic tests
For basic tests, need to support multiple inputs in various commands
is unnecessary.  Remove the support and adjust tests appropriately.
test/kvs: Remove user 'convenience' input
Remove convenience option where no key means looking at the root
directory (i.e. ".").  This convenience isn't necessary for testing.

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch from 4e6530c to d68c69f Dec 20, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+0.02%) to 76.353% when pulling d68c69f on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+0.02%) to 76.349% when pulling d68c69f on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch from d68c69f to 9e9063f Dec 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.02%) to 76.353% when pulling 9e9063f on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch from 9e9063f to 00e820e Dec 21, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.06%) to 76.391% when pulling 00e820e on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch 2 times, most recently from 2d03bfb to b5dd696 Dec 21, 2016

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 21, 2016

Ended up there was some small racey behavior in my tests b/c of a flux-kvs watch background process racing with foreground tasks. Should be solved now.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.03%) to 76.361% when pulling b5dd696 on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.06%) to 76.391% when pulling b5dd696 on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch from b5dd696 to 03245f3 Dec 21, 2016

chu11 added some commits Dec 9, 2016

doc/flux-kvs(1): Remove deleted commands
Remove get-treeobj, put-treeobj, getat, dirat, readlinkat, dirsize,
type, copy-tokvs, and copy-fromkvs.
cmd/flux-kvs: Alter watch and watchdir logic
Adjust logic of watch and watchdir commands to output 'count' changes
and not 'count' versions.
cmd/flux-kvs: Add -o option to watch and watchdir
Add -o option to watch and watchdir that will output the current
version stored in the key before watching.
doc/flux-kvs(1): Update watch/watch-dir commands
Update watch and watch-dir commands w/ recent changes (-o option
and logic of output).
doc/flux-kvs(1): Update multi-input documentation
Update 'mkdir' and 'readlink' documentation to indicate multiple
keys can be specified.
cmd/flux-kvs(1): Change -r to -R
For consistency to many coreutils tools (ls, rm, etc.), change
-r option to -R option to specify recursion.
cmd/flux-kvs: Make 'count' an option
In 'watch' and 'watch-dir' subcommands, make the 'count' argument
an option instead of an argument.
cmd/flux-kvs: Merge dropcache commands
Merge dropcache and dropcache-all commands into one with new
--all option.
cmd/flux-kvs(1): Handle errors in unlink
Handle error situations in unlink subcommand, such as when user
attempts to remove non-existent KVS entries or non-empty directories.
Support -R option to unlink to recursively delete non-empty directories.
cmd/flux-kvs: Merge watch and watch-dir
Merge watch and watch-dir commands into a single command.
Handle many additional corner cases, such as entries being
deleted or converting between keys and directories.
doc/flux-kvs(1): Minor re-organizing
Reorder subcommand listing to be consistent to new flux-kvs command
and be ordered by usability.

@chu11 chu11 force-pushed the chu11:kvscleanup2 branch from affbbf7 to 0702878 Dec 22, 2016

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 22, 2016

Just pushed, also added 1 extra patch re-organizing the ordering of subcommands in the manpage.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 22, 2016

Coverage Status

Coverage increased (+0.02%) to 76.353% when pulling 0702878 on chu11:kvscleanup2 into a1fe845 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 22, 2016

hmmm, first time through a test failed on one build job b/c of the first test in t0007-ping.t, which I can't imagine was related to anything in this PR.

test_expect_success 'ping: 10K 1K byte echo requests' '
        run_timeout 5 flux ping --pad 1024 --count 10240 --interval 0 0
'

I'm going to guess the 1/10th of the core this is running on got scheduled poorly and this thing timed out due to the large number of pings. Maybe should bump up the timeout if we see this anymore.

Rebuilt and things all passed this time.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 22, 2016

Yup, looks good. Thanks again @chu11! Merging.

@garlick garlick merged commit 24973c8 into flux-framework:master Dec 22, 2016

4 checks passed

codecov/patch 78.68% of diff hit (target 76.03%)
Details
codecov/project 76.08% (+0.05%) compared to a1fe845
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 76.353%
Details

@garlick garlick removed the review label Dec 22, 2016

@garlick garlick referenced this pull request Dec 28, 2016

Closed

flux-kvs needs cleanup #897

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

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.