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

Misc Refactoring & Cleanup in libkvs #1153

Merged
merged 6 commits into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Aug 18, 2017

Spliced out patches from a larger RFC11 PR that @garlick and I are working on. Apologies I missed these and didn't put them in #1152 as I couldn't quite see their independence at first.

All patches are from @garlick. I've reviewed and it looks good to me. But as I'm opening this PR, think it's wrong for me to hit the button.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 18, 2017

One thing I'd suggest is to add more verbose descriptions in the commit body for these commits.

libkvs/kvs_txn: Refactor kvs_txn
Splice flux_kvs_fence() and flux_kvs_commit() out from kvs_txn
and into new kvs_commit files.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.883% when pulling 5e2ea3c on chu11:misccleanup8 into 0337f01 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:misccleanup8 branch from 5e2ea3c to c80bd76 Aug 18, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 18, 2017

Codecov Report

Merging #1153 into master will decrease coverage by 0.05%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
- Coverage   77.54%   77.49%   -0.06%     
==========================================
  Files         157      158       +1     
  Lines       28751    28756       +5     
==========================================
- Hits        22295    22284      -11     
- Misses       6456     6472      +16
Impacted Files Coverage Δ
src/common/libkvs/kvs_txn.c 58.69% <ø> (-2.46%) ⬇️
src/common/libkvs/kvs_dir.c 79.72% <100%> (+0.85%) ⬆️
src/common/libkvs/kvs_lookup.c 84.21% <100%> (+2.96%) ⬆️
src/common/libkvs/kvs_commit.c 86.66% <86.66%> (ø)
src/common/libflux/keepalive.c 86.66% <0%> (-6.67%) ⬇️
src/broker/module.c 83.28% <0%> (-1.4%) ⬇️
src/broker/content-cache.c 73.27% <0%> (-1.3%) ⬇️
src/common/libflux/mrpc.c 85.15% <0%> (-1.18%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
src/common/libkvs/kvs_watch.c 86.47% <0%> (-0.97%) ⬇️
... and 12 more
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 18, 2017

@grondo Agreed, they were a bit too simple. Updated with more details.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 18, 2017

Doh, travis caught some warnings that gcc didn't catch for either of our boxes I guess.

errno == EINVAL;

did get one weird failure

FAIL: t0016-cron-faketime.t 5 - flux-cron tab works for Mondays, midnight

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

I'll assume the error was due to something inside the travis system. Re-push & try again.

@chu11 chu11 force-pushed the chu11:misccleanup8 branch from c80bd76 to 306a7aa Aug 18, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage increased (+0.0008%) to 77.901% when pulling 306a7aa on chu11:misccleanup8 into 0337f01 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 18, 2017

Had a couple more "write error" builds, had to restart them. But got one with:

FAIL: t0001-basic.t 41 - broker --k-ary option works

in the raw logs

flux-start: warning: setting --bootstrap=selfpmi due to --size option
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'content-sqlite' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'job' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'job' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'job' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'job' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-start: warning: setting --bootstrap=selfpmi due to --size option
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'content-sqlite' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-start: warning: setting --bootstrap=selfpmi due to --size option
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'barrier' was not cleanly shutdown
flux-broker: module 'resource-hwloc' was not cleanly shutdown
flux-broker: module 'kvs' was not cleanly shutdown
flux-broker: module 'aggregator' was not cleanly shutdown
flux-start: 0 (pid 77952) Killed
not ok 41 - broker --k-ary option works

Perhaps #1126 ? Gonna restart it.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 18, 2017

codecov diff is sort of bad, I"ll try and add some simple unit tests to catch bad input checks tomorrow

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage decreased (-0.03%) to 77.868% when pulling 9d4ad17 on chu11:misccleanup8 into 0337f01 on flux-framework:master.

if (!(f = flux_kvs_fence (h, flags, zuuid_str (uuid), 1, txn)))
goto done;
done:
saved_errno = errno;

This comment has been minimized.

@grondo

grondo Aug 18, 2017

Contributor

Could this function could be simplified (and the confusing goto done when the done: label is on the next line removed) with something like this?

    if (!(uuid = zuuid_new ()) || !(f = flux_kvs_fence (h, flags, zuuid_str (uuid), 1, txn)))
        saved_errno = errno;
    zuuid_destroy (&uuid);
    errno = saved_errno;
    return f;

Also avoids setting errno to assign it to saved_errno, and set it back to errno again.

If taking this approach it might be good to also initialize saved_errno. (then you could also add something like if (saved_errno) errno = saved_errno;)

This comment has been minimized.

@chu11

chu11 Aug 18, 2017

Author Contributor

that would simplify it a bit.

return NULL;
}
if (validate_lookup_flags (flags) < 0)
return NULL;
if (!treeobj) {

This comment has been minimized.

@grondo

grondo Aug 18, 2017

Contributor

just an idle suggestion -- a couple lines of code could be removed by combining the two conditionals here:

    if (!h || !key || strlen (key) == 0 || validate_lookup_flags (flags) < 0) {
        errno = EINVAL;
        return NULL;
    }

as a bonus validate_lookup_flags doesn't have to set its own errno (it is internal and only fails in one way anyhow)

This comment has been minimized.

@chu11

chu11 Aug 18, 2017

Author Contributor

i like this idea

@chu11 chu11 force-pushed the chu11:misccleanup8 branch from 9d4ad17 to 1284050 Aug 18, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage decreased (-0.01%) to 77.886% when pulling 1284050 on chu11:misccleanup8 into 0337f01 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 18, 2017

Typo in commit message for 1c26ce4? "Simply" should be "Simplify"?

Also, maybe add a descriptive message to 1284050, or perhaps squash it into another commit? (Sorry didn't quite grok if the code was just moved around, in which case it might make more sense to keep your cleanup commit separate). If you feel the cleanup is simple enough to not need a message body, though, I'd be ok with that.

Otherwise, is this ready? Nice work on coverage of added code 👍

garlick and others added some commits Jul 27, 2017

libkvs/commit: Cleanup flux_kvs_fence
Simplify handling of NULL transaction in flux_kvs_fence().
libkvs/lookup: Add parameter checks
Add parameter checks to flux_kvs_lookup() and flux_kvs_lookupat().
libkvs/test: Add simple error check tests
Add simple error check tests for kvs_lookup and kvs_dir.

@chu11 chu11 force-pushed the chu11:misccleanup8 branch from 1284050 to 6c86b3c Aug 18, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Aug 18, 2017

@grondo Doh, thanks for the typo catch.

Yeah, not really much to say about 1284050, as it's just moving code around. I changed the commit log say "simplify" instead of "cleanup", since that may make the single line make more sense. I didn't want to squash it into the earlier commit de37f36, b/c that one was more about splitting the functions out of the kvs_txn file.

Basically it's all ready for merge.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage decreased (-0.05%) to 77.85% when pulling 6c86b3c on chu11:misccleanup8 into 0337f01 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Aug 18, 2017

Ok, thanks.

Had to restart one of the builds due to write error -- argh that is annoying.

@grondo grondo merged commit 0c63795 into flux-framework:master Aug 18, 2017

4 checks passed

codecov/patch 91.66% of diff hit (target 77.54%)
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +14.12% compared to 0337f01
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 77.85%
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.