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 Jansson Updates #1108

Merged
merged 26 commits into from Jul 21, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Jul 18, 2017

This PR converts all the code in the kvs to use jansson instead of json-c. This PR should be done AFTER @garlick's #1107 PR, but wanted to throw this up here for the time being.

Still need to valgrind check and soak test too. Maybe will add 1-2 unit tests for a code path I think should be tested.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 18, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.358% when pulling abc0595 on chu11:kvs_jansson_try3 into a3add43 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jul 18, 2017

Nice! Do we need the proto.[ch] after this at all?

I guess this won't git bisect but that would be quite a bit of extra work so IMHO probably not worth it.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 18, 2017

Not sure if we can remove proto.[ch] yet, wanted to see how much you did in #1107. But it's minimally close. I think there's one test that still uses it, so perhaps we'd have to convert that. Also need to figure out where the KVS_PROTO_* enums would go (maybe you already did this, still need to look through your PR).

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jul 18, 2017

#1107 removes the client API users of proto.[ch]. Ah right, t/kvs/watch_disconnect.c uses it, but that's easily fixed.

As I needed each of the KVS_PROTO_* flags, I defined new ones (same values, different names) in API header files. Since the KVS module includes <flux/core.h> it can pick them up and use the new names.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 18, 2017

something I just thought about w/ json_compare() which uses json_equal(). It doesn't behave like the old algorithm. If one were to pass json_compare (NULL, NULL), it would now return false.

Now under jansson, this might be what we want. Hmmm, minimally should probably just add a comment about this.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jul 18, 2017

Just peeked at watch_request_cb(), the one user of json_compare(), to try to figure out if NULL, NULL ever came up. I think it can, e.g. watching a key that initially, or at some point while it is being watched, doesn't exist.

The watch handling really needs a redesign, but let's not do that here.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 18, 2017

@garlick I think I protect against this in watch_request_cb. If the value isn't found in lookup, I convert to a json null.

    val = lookup_get_value (lh);
    /* if no value, create json null object for remainder of code */
    if (!val) {
        if (!(val = json_null ()))
            oom ();
    }

when unpacking the msg, I think a "null" will be converted to a json null.

        if (flux_request_unpack (msg, NULL, "{ s:s s:o s:i }",
                                 "key", &key,
                                 "val", &oval,
                                 "flags", &flags) < 0) {
            flux_log_error (h, "%s: flux_request_unpack", __FUNCTION__);
            goto done;
        }

So I think val & oval will always be non-NULL. As a test I added an assert (val && oval) right before and the unit tests pass.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jul 18, 2017

That seems reasonable then.

I double-checked my changes to the kvs_watch_once() implementation. It passes in a JSON null for "val" if the initial json_str value is NULL, so we are consistent there.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 19, 2017

Just ran some soak tests, and the numbers were a tad better than master. So looks like jansson is having a nice positive effect :-)

@chu11 chu11 force-pushed the chu11:kvs_jansson_try3 branch from abc0595 to f89b77d Jul 19, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 19, 2017

Codecov Report

Merging #1108 into master will decrease coverage by 0.07%.
The diff coverage is 80.41%.

@@            Coverage Diff             @@
##           master    #1108      +/-   ##
==========================================
- Coverage   78.06%   77.98%   -0.08%     
==========================================
  Files         159      157       -2     
  Lines       26220    25988     -232     
==========================================
- Hits        20469    20268     -201     
+ Misses       5751     5720      -31
Impacted Files Coverage Δ
src/common/libkvs/jansson_dirent.c 80.55% <ø> (+5.55%) ⬆️
src/modules/kvs/kvs.c 77.1% <71.42%> (-2.01%) ⬇️
src/modules/kvs/cache.c 92.64% <75%> (-1.25%) ⬇️
src/modules/kvs/kvs_util.c 76.19% <78.94%> (ø)
src/modules/kvs/fence.c 80.88% <81.48%> (-4.37%) ⬇️
src/modules/kvs/commit.c 85.31% <88.33%> (-2.34%) ⬇️
src/modules/kvs/lookup.c 91.73% <93.87%> (-0.61%) ⬇️
src/broker/modservice.c 79.2% <0%> (-1%) ⬇️
src/common/libutil/shortjson.h 90.8% <0%> (-0.69%) ⬇️
src/common/libutil/dirwalk.c 94.07% <0%> (ø) ⬆️
... and 6 more

@chu11 chu11 force-pushed the chu11:kvs_jansson_try3 branch from f89b77d to cff8b1f Jul 19, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 19, 2017

Just pushed a branch rebased on current master (which includes #1107). Mostly cleanup patches, adjusting for new enums in master, added more commit API unit tests, fix corner case in t1002-kvs-cmd.t tests b/c of potential ordering issues with kvs output.

Now that I've rebased, I'm sure there is some additional cleanup can be done, which I'll look into now. Perhaps proto can go away, maybe json_dirent, etc.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.198% when pulling cff8b1f on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 19, 2017

hmmmm, I'm not really sure why travis doesn't like my unit tests. No unit tests themselves fail, but it does give up after awhile with a missing test plan error.

ERROR: t1002-kvs-cmd.t - missing test plan
ERROR: t1002-kvs-cmd.t - exited with status 1

It's around where I put a new function in in cff8b1f dunno if it's some subtle-bash-ism or something. Maybe they want my helper function to return 0?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 19, 2017

That error usually happens when the test script exits before test_done is called.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2017

Coverage Status

Coverage decreased (-0.08%) to 78.221% when pulling 23405c9 on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 19, 2017

Coverage Status

Coverage decreased (-0.07%) to 78.228% when pulling 40409d6 on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Coverage decreased (-0.09%) to 78.209% when pulling b92a669 on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 20, 2017

Hmmm, having issues passing travis. I'm sort of at a loss for the moment. This relatively simple change:

+# Output of watch_out could be unsorted/out of order compared to
+# expected output.  This function will re-order the output in each
+# appropriate section (i.e. between ====================== lines)
+sort_watch_output() {
+        i=0
+        rm -f watch_out_sorted
+        while read LINE
+        do
+            if [ "$LINE" == "======================" ]
+            then
+                IFS=$'\n'
+                arr2=($(printf "%s\n" "${arr[@]}" | sort))
+                for elem in "${arr2[@]}"
+                do
+                    echo "$elem" >> watch_out_sorted
+                done
+                echo "======================" >> watch_out_sorted
+                unset IFS
+                i=0
+            else
+                arr[$i]="$LINE"
+                i=$((i + 1))
+            fi
+        done < watch_out
+}
+
 test_expect_success 'kvs: watch a dir with -R'  '
         flux kvs unlink -R $DIR &&
         flux kvs put $DIR.a.a=0 $DIR.a.b=0 &&
@@ -572,6 +598,7 @@ test_expect_success 'kvs: watch a dir with -R'  '
         wait_watch_current "======================" &&
         flux kvs put $DIR.a.a=1 &&
         wait $watchpid
+        sort_watch_output
        cat >expected <<-EOF
        $DIR.a.a = 0
        $DIR.a.b = 0
@@ -580,7 +607,7 @@ test_expect_success 'kvs: watch a dir with -R'  '
        $DIR.a.b = 0
        ======================
        EOF
-        test_cmp watch_out expected
+        test_cmp watch_out_sorted expected
 '

seems to be confusing the test and giving me

ERROR: t1002-kvs-cmd.t - missing test plan
ERROR: t1002-kvs-cmd.t - exited with status 1

Which as @grondo says above, is usually indication that the test has exited early without calling test_done. I initially thought I was doing some advanced bash-ism and maybe travis's bash is old. But arrays have been around since bash 2.0. I thought maybe unsetting IFS was bad, but that didn't seem to change any results either.

I'm also regularly getting build failures on one of the travis clang builds w/ cppcheck, but I'm at a bit of a loss.

[src/modules/kvs/test/json_util.c:103]: (error) Memory pointed to by 's1' is freed twice.
[src/modules/kvs/test/json_util.c:114]: (error) Memory pointed to by 's1' is freed twice.

But my code seems correct.

    93	    obj = json_null ();
    94	
    95	    ok ((s1 = json_strdump (obj)) != NULL,
    96	        "json_strdump works");
    97	
    98	    s2 = "null";
    99	
   100	    ok (!strcmp (s1, s2),
   101	        "json_strdump works on null object");
   102	
   103	    free (s1);
   104	    json_decref (obj);
   105	
   106	    ok ((s1 = json_strdump (NULL)) != NULL,
   107	        "json_strdump works on NULL pointer");
   108	
   109	    s2 = "null";
   110	
   111	    ok (!strcmp (s1, s2),
   112	        "json_strdump works on NULL pointer");
   113	
   114	    free (s1);

minimally for the latter free (s1), it's not possible it is freed twice, since I set s1 on line 106 above and never use s1 again. Unless the json object (obj) owns the string? But that's not the case according to the jansson API docs, and I free the string a gazillion of other places too. This would be a problem elsewhere if it were true.

Oh well, will mull over it, maybe will have an epiphany in my sleep.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.187% when pulling d847827 on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jul 20, 2017

Hmm, under the assumption that cppcheck was confused I just cleared the s1 values before re-using and that worked.

index 729b4d0..2d81387 100644
--- a/src/modules/kvs/test/json_util.c
+++ b/src/modules/kvs/test/json_util.c
@@ -89,6 +89,7 @@ int main (int argc, char *argv[])
 
     free (s1);
     json_decref (obj);
+    s1 = NULL;
 
     obj = json_null ();
 
@@ -102,6 +103,7 @@ int main (int argc, char *argv[])
 
     free (s1);
     json_decref (obj);
+    s1 = NULL;
 
     ok ((s1 = json_strdump (NULL)) != NULL,
         "json_strdump works on NULL pointer");
@@ -112,6 +114,7 @@ int main (int argc, char *argv[])
         "json_strdump works on NULL pointer");
 
     free (s1);
+    s1 = NULL;
 
     done_testing ();
     return (0);
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Coverage decreased (-0.08%) to 78.224% when pulling a4a15e7 on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

@flux-framework flux-framework deleted a comment from coveralls Jul 20, 2017

@flux-framework flux-framework deleted a comment from coveralls Jul 20, 2017

@flux-framework flux-framework deleted a comment from coveralls Jul 20, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 20, 2017

Coverage Status

Coverage decreased (-0.08%) to 78.221% when pulling 6694838 on chu11:kvs_jansson_try3 into 500e366 on flux-framework:master.

chu11 and others added some commits Jul 19, 2017

modules/kvs: Refactor build of unit tests
Only build tests with the object files they require, instead
of using all object files in the directory.  This will make it
easier for testing as unit tests are updated one at a time.

Eliminate unnecessary header inclusion in unit tests.
modules/kvs: Add comments & unit tests
Add comments and unit tests to indicate json_compare() logic when
a NULL pointer may be passed into it.
modules/kvs: Convert lookup API to use jansson
Conversion also requires elimination of use of proto.h.  Convert
KVS_PROTO_READDIR, KVS_PROTO_READLINK, and KVS_PROTO_TREEOBJ
to FLUX_KVS_READDIR, FLUX_KVS_READLINK, and FLUX_KVS_TREEOBJ
respectively.
modules/kvs: Update lookup tests for jansson
Conversion also requires elimination of use of proto.h.  Convert
KVS_PROTO_READDIR, KVS_PROTO_READLINK, and KVS_PROTO_TREEOBJ
to FLUX_KVS_READDIR, FLUX_KVS_READLINK, and FLUX_KVS_TREEOBJ respectively.
modules/kvs: Add json_strdump util function
Add util function so different parts of KVS that need to call
json_dumps() call it consistently.
modules/kvs: Convert kvs to use jansson
Convert kvs to use jansson.  Use pack/unpack functions
appropriately as needed to remove dependency on json-c
functions.

Conversion also requires elimination of use of proto.h.
Convert KVS_PROTO_FIRST and KVS_PROTO_ONCE to KVS_WATCH_FIRST
and KVS_WATCH_ONE appropriately.
modules/kvs: Add more unit tests to commit tests
Add tests checking for deletes of invalid keys.
Add tests for multiple fences and merged/unmerged multiple fences.
Add test for giant directory.
t/t1002-kvs-cmd.t: Fix testing corner case
In some watch tests, the output can be in unsorted order, leading
to a mismatch with "expected" output.  Add a small tweak to sort
fields from the output before comparison.
common/libkvs: Remove proto.[ch]
With modules/kvs conversion to jansson, proto.[ch] files and functions
are no longer used anywhere in flux-core.

Also remove lingering build dependency on proto.o in t/Makefile.am.
common/libkvs: Remove json_dirent.[ch]
With modules/kvs conversion to jansson, json_dirent.[ch] files
and functions are no longer used anywhere in flux-core.
modules/kvs: workaround cppcheck false positive
cppcheck detects "Memory pointed to by 's1' is freed twice".

This is believed to be false positive.  Set s1 to NULL after free
to workaround it.
libkvs/jansson_dirent: drop j_dirent_append()
The flux_kvs_txn_t object now abstracts the "op array"
passed in a commit/fence request.  Drop j_dirent_append()
which operates on these arrays.

Remove it from the jansson_dirent unit test as well.
modules/kvs: Remove json_compare
With jansson conversion, json_compare() is now simply a call
to json_equal().  Remove json_compare() everywhere and replace
with direct call to json_equal().
libkvs/jansson_dirent: Remove j_dirent_match
With jansson conversions in modules/kvs and common/libkvs,
j_dirent_match() was no longer used except in jansson_dirent
unit tests.

Remove function globally and use json_equal() in unit tests
instead.
modules/kvs: Re-work commit unit tests
Create own convenience function for unit tests instead of
using j_dirent_append().
modules/kvs: Rename json_util functions
With use of jansson, some of the json_util function names may
appear to be part of the jansson library.  To differentiate the
functions, adjust names and have all of them prefixed with "kvs_util".

Rename files appropriately, use appropriate new header file, and
use new functions where appropriate.

@chu11 chu11 force-pushed the chu11:kvs_jansson_try3 branch from 68d37e7 to f146b1c Jul 20, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 21, 2017

Coverage Status

Coverage decreased (-0.08%) to 78.256% when pulling f146b1c on chu11:kvs_jansson_try3 into 0673cf2 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jul 21, 2017

I'm going to press the button! Nice work.

@garlick garlick merged commit fced5eb into flux-framework:master Jul 21, 2017

4 checks passed

codecov/patch 80.41% of diff hit (target 78.06%)
Details
codecov/project Absolute coverage decreased by -0.07% but relative coverage increased by +2.35% compared to 0673cf2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 78.256%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.