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

kvs: support append operation #1265

Merged
merged 9 commits into from Nov 1, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Oct 30, 2017

This PR implements basic append support in the KVS and a basic --append option in flux-kvs.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 30, 2017

Codecov Report

Merging #1265 into master will decrease coverage by 0.06%.
The diff coverage is 72.41%.

@@            Coverage Diff            @@
##           master   #1265      +/-   ##
=========================================
- Coverage   77.96%   77.9%   -0.07%     
=========================================
  Files         154     154              
  Lines       29018   29093      +75     
=========================================
+ Hits        22624   22664      +40     
- Misses       6394    6429      +35
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 79.9% <100%> (+0.16%) ⬆️
src/common/libkvs/kvs_txn.c 75.28% <100%> (+1.26%) ⬆️
src/common/libkvs/treeobj.c 84.47% <57.14%> (+0.51%) ⬆️
src/modules/kvs/commit.c 74.66% <72.61%> (-1.87%) ⬇️
src/broker/module.c 83.79% <0%> (-1.4%) ⬇️
src/modules/connector-local/local.c 76.07% <0%> (-1.31%) ⬇️
src/broker/content-cache.c 73.43% <0%> (-1.3%) ⬇️
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/common/libkvs/kvs_watch.c 87.55% <0%> (-0.89%) ⬇️
... and 9 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 30, 2017

Coverage Status

Coverage decreased (-0.05%) to 78.539% when pulling 0994d1b on chu11:issue1193-part1 into 3a286cb on flux-framework:master.

@garlick
Copy link
Member

garlick left a comment

Wow you got that done quick! Nice!
Couple minor comments inline.

@@ -122,7 +122,7 @@ int flux_kvs_txn_put_raw (flux_kvs_txn_t *txn, int flags,
errno = EINVAL;
goto error;
}
if (validate_flags (flags, 0) < 0)
if (validate_flags (flags, FLUX_KVS_APPEND) < 0)

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

We should allow append in flux_kvs_txn_put() also.

@@ -7,6 +7,10 @@ extern "C" {

#include <stdarg.h>

enum {

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

Just noticed all the flags in kvs.h are now lookup specific. I moved them into kvs.h because earlier we alloed the FLUX_KVS_TREEOBJ flag with txn_put.

I guess I'd vote we put all the KVS user flags in the same enum in kvs.h unless others feel they should be split up.

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

ahh, sounds good to put it all in kvs.h

return -1;
}
else {
char *s = json_dumps (entry, 0);

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

use treeobj_encode()?

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

Since this is the "corrupt treeobj" path, I think using json_dumps() is the right idea.

Using treeobj_encode() works right now b/c it presently just calls json_dumps() directly and does nothing else. Could treeobj_encode() ever check for treeobj_validate?

Store 'value' under 'key' and commit it. If it already has a value,
overwrite it. If no options, value is stored directly. If '-j', it is
first encoded as JSON, then stored. If '-r', the value may be read from
standard input if specified as "-", and may include embedded NULL bytes.
If '-t', value is stored as a RFC 11 object. '-n' prevents the commit
from being merged with with other contemporaneous commits.
from being merged with with other contemporaneous commits. '-A' appends the
value to a key instead of overwriting the value. Append only works with the

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

should work with no options, if my suggestion is accepted. JSON is the one that gets weird since a JSON object appended to a JSON object isn't valid JSON.

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

Ahh, I didn't adjust given changes with #1262. Will have to go back and change that.

@@ -94,6 +94,9 @@ static struct optparse_option put_opts[] = {
{ .name = "no-merge", .key = 'n', .has_arg = 0,
.usage = "Set the NO_MERGE flag to ensure commit is standalone",
},
{ .name = "append", .key = 'A', .has_arg = 0,

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

Any reason not to use little a for the short opt?

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

little a is used for the "at" option throughout flux kvs, so picked big A just to be different.

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

Oh duh.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 30, 2017

repushed with minor fixes as discussed above. Fixed a major bug I found. Required refactoring of treeobj_copy_dir() into treeobj_copy() and working w/ valrefs and dirrefs too.

@@ -57,8 +57,8 @@ first encoded as JSON, then stored. If '-r', the value may be read from
standard input if specified as "-", and may include embedded NULL bytes.
If '-t', value is stored as a RFC 11 object. '-n' prevents the commit
from being merged with with other contemporaneous commits. '-A' appends the
value to a key instead of overwriting the value. Append only works with the
'-r' option.
value to a key instead of overwriting the value. Append works with no options

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

Maybe simpler to say

Append is incompatible with the -j option.

*/
json_t *treeobj_copy_dir (json_t *obj);
json_t *treeobj_copy (json_t *obj);

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

Should this be named so that it's more clear that it makes a thin copy of the data object?

This property is critical to understanding how commits are processed.

Maybe treeobj_copy_thin() or similar?

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

Actually it is a deep copy as long as it's not a directory that includes "rolled up" subdirectories. Hmm, maybe just need to add a new treeobj_copy() that makes a deep copy and leave that other one alone?

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

I can see your point that treeobj_copy() by itself may be ambiguous. I had wanted to avoid it, perhaps adding separate treeobj_copy_dirref() and treeobj_copy_valref() functions would be better?

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

How about treeobj_copy() that fully copies any treeobj, and then rename treeobj_copy_dir() to something more indicitave of what it's doing like treeobj_copy_dir_thin()? (the rename could be deferred to another PR)

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

Now that I think about it, how about just making a function treeobj_copy_shallow(). It can be made quite clear that it's a shallow copy of the data within a treeobj, regardless of the data it is.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 30, 2017

Good job finding that bug!

Maybe the json objects returned from the cache should be tagged const ?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 30, 2017

good idea on making the cache returns const. I'll instead add that to the list of cleanups in #1264.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 30, 2017

Don't forget to update the FLAGS section of flux_kvs_txn_create.adoc with a description of FLUX_KVS_APPEND.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 30, 2017

just pushed with smaller incremental changes as discussed above. Also completely redid the prior treeobj_copy_dir() refactor. Instead, a new function treeobj_copy() does deep copies on all tree objs.

@@ -263,6 +263,84 @@ int treeobj_insert_entry (json_t *obj, const char *name, json_t *obj2)
return 0;
}

json_t *treeobj_copy (json_t *obj)

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

would json_deep_copy() work here?

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

naming wise? That would follow jansson style naming. Are you thinking treeobj_copy() would be the shallow equivalent?

This comment has been minimized.

@garlick

garlick Oct 30, 2017

Member

I meant could the jansson function json_deep_copy() do what treeobj_copy() proposes to do here?

http://jansson.readthedocs.io/en/2.8/apiref.html#copying

This comment has been minimized.

@chu11

chu11 Oct 30, 2017

Author Contributor

ohhh, duh! You're right. I'm going to rename to treeobj_deep_copy() as well, as that would follow the jansson style. treeobj_copy() will be the treeobj equivalent of shallow copy.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

just re-pushed, decided to rename functions, so now we have treeobj_copy() which is a treeobj shallow copy, and treeobj_deep_copy(), which is a deep copy.

Also noticed a corner case in treeobj_create_symlink(), so I added a check and unit test there.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

I occurred to me on the drive home. Given how I implemented treeobj_copy() as a shallow copy of the data within a treeobject, treeobj_deep_copy() is no longer needed. Should we keep it around as a potentially useful future function? Or axe it.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 31, 2017

Coverage Status

Coverage decreased (-0.07%) to 78.519% when pulling fcb92f4 on chu11:issue1193-part1 into 3a286cb on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 31, 2017

Given how I implemented treeobj_copy() as a shallow copy of the data within a treeobject, treeobj_deep_copy() is no longer needed. Should we keep it around as a potentially useful future function? Or axe it.

I may be getting lost here. Why not just have a treeobj_copy() that directly calls json_deep_copy(), and the original treeobj_copy_dir() (possibly renamed) with its specialized function for commit processing? I'm not sure why treeobj_copy() needs to be type specific.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

I don't think we're on the same page on this. I'll drop by when I get in.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 31, 2017

Sounds good :-)

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

just re-pushed, cleanup treeobj_copy() by calling json_deep_copy(). Also nit cleanup in treeobj_deep_copy().

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

Ugh ...I forgot to commit --amend something. tiny re-push.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 31, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.487% when pulling b93e9af on chu11:issue1193-part1 into 3a286cb on flux-framework:master.

common/libkvs: Add error check
Add input error check ot treeobj_create_symlink() with appropriate
unit test.
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 31, 2017

OK, ready to merge?

chu11 added some commits Oct 31, 2017

common/libkvs: Add treeobj_deep_copy()
Add new function to support deep copy of treeobjs.
common/libkvs: Refactor treeobj_copy_dir()
Change to treeobj_copy() and make function a shallow copy for
all treeobj types.

Update unit tests appropriately.

Update callers appropriately.
common/libkvs/txn: Add FLUX_KVS_APPEND flag
Update txn_encode_op, flux_kvs_txn_put, and flux_kvs_txn_put_raw
to allow user to set FLUX_KVS_APPEND flag.

Add unit tests for corner case tests to bad input in txn_encode_op.

Update documentation appropriately.
modules/kvs/commit: Refactor internal structure
Split item_callback_list into two lists, missing_refs_list which
will hold missing references to look up when user calls
commit_iter_missing_refs() and and dirty_cache_entries_list
which will have dirty cache entries to flush when user calls
commit_iter_dirty_cache_entries().
modules/kvs/commit: Refactor for flags option
In both commit API and commit unit tests, pass around/set
flags argument from commit operations.
modules/kvs/commit: Support commit append
Support ability to append to already existing KVS values.

Add new append unit tests.

@chu11 chu11 force-pushed the chu11:issue1193-part1 branch from b93e9af to 0145b56 Oct 31, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

just squashed and re-pushed. Two tiny tweaks in the squash.

Removed calls to treeobj_validate() in treeobj_copy() and treeobj_deepcopy() as they were considered superfluous.

Also removed a few unit tests for treeobj_deep_copy(), as they were a bit superfluous given the tests that existed for treeobj_copy().

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 31, 2017

Looks good @chu11 - will merge once travis passes.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 31, 2017

Coverage Status

Coverage decreased (-0.07%) to 78.515% when pulling 0145b56 on chu11:issue1193-part1 into 3a286cb on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Oct 31, 2017

one build hung after

PASS: t4000-issues-test-driver.t 4 - t0900-wreck-invalid-cores.sh

restarting.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 1, 2017

hit

ERROR: t2001-jsc.t - exited with status 137 (terminated by signal 9?)

i'm assuming slowness and a broker had to be killed. restarting build.

@garlick garlick merged commit 00b06d1 into flux-framework:master Nov 1, 2017

2 of 4 checks passed

codecov/patch 72.41% of diff hit (target 77.96%)
Details
codecov/project 77.9% (-0.07%) compared to 3a286cb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 78.515%
Details

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.