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

Support KVS no merge flag #982

Merged
merged 9 commits into from Feb 21, 2017

Conversation

Projects
None yet
6 participants
@chu11
Copy link
Contributor

chu11 commented Feb 17, 2017

Here's an initial try at issue #813. Note that I duplicated a bunch of functions by appending _flags to the name (e.g. kvs_commit_flags()). I didn't want to break ABI for the time being. Will convert appropriately later on. In hindsight, I think I should have two commits, one introducing a "flags", then another introducing the flag KVS_NO_MERGE. Will redo that when I cleanup.

I elected to the make the "mergeable" boolean in each fence wire packet optional. I can sort of go either way on this. It could be made a mandatory boolean in each packet?

Probably need some additional tests too, minimally to test flags on the fence functions.

Wasn't sure how to go about the ABI breakage. Obviously I can fix everything in flux-core, but should I just send PR to flux-sched as well at the appropriate time?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.02%) to 76.227% when pulling fa3d15d on chu11:issue813-kvsnomergeflag into b877c17 on flux-framework:master.

@lipari

This comment has been minimized.

Copy link
Contributor

lipari commented Feb 17, 2017

but should I just send PR to flux-sched as well at the appropriate time?

Yes, please.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 17, 2017

I got lost in #813, so sorry if this should be obvious, but is the KVS_NO_MERGE flag only for use in testing?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 17, 2017

This is cool!

I haven't had a chance to review in detail but my first thought is to go with integer flags in both API and wire protocol, and to avoid optional JSON fields (personal pref, but it tends to make encode/decode slightly more complicated when they are optional)

I second @lipari's comment - what we have done in the past is to submit a sched PR with an "apply after flux-core pr number whatever is merged" note.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 17, 2017

is the KVS_NO_MERGE flag only for use in testing?

Sorry, #813 was kind of all over the place and I got lost at one point too. I think the answer is no.

This seems to me to be a useful way to allow the commit merging optimization to remain for the common case, but provide a way to disable it on a commit basis for special cases, e.g. the "state machine tracking" case that came up before.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 17, 2017

This seems to me to be a useful way to allow the commit merging optimization to remain for the common case, but provide a way to disable it on a commit basis for special cases, e.g. the "state machine tracking" case that came up before.

Sorry, another dumb question: Just to make sure I understand, will this work for multiple handles doing kvs commits, e.g. if I want to disable commit merging within my handle (for a specific commit), but in the meantime some other actor within an instance calls kvs_commit without this flag one or more times?

I'd feel better if there was a test case for that if possible

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 17, 2017

will this work for multiple handles doing kvs commits, e.g. if I want to disable commit merging within my handle (for a specific commit), but in the meantime some other actor within an instance calls kvs_commit without this flag one or more times

The flag should protect your commit from being merged with any other commit in the master (regardless of where the other commits originate).

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 17, 2017

Great. Thanks for the clarification.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 17, 2017

@garlick Ahh, I now see that you used an integer "flags" in the wire protocol for kvs get. I'll follow the pattern used there.

chu11 added some commits Feb 17, 2017

modules/kvs: Add flags to commit/fence functions
Add a flags argument to kvs_commit, kvs_commit_begin, kvs_fence,
and kvs_fence_begin functions.

Fix ABI across all of remaining flux-core.
modules/kvs: Add flags to commit/fence internals
Add commit/fence flags internally to wire protocol and internal
fence data structures.
modules/kvs/test: Update tests for flags
Update tests for flags arguments in kvs wire protocol.
modules/kvs: Support KVS_NO_MERGE flag
Support kvs flag KVS_NO_MERGE in commit and fence functions to
inform KVS to not merge commits with other ones when commits
are finalized.

Fixes #813
test/kvs: Add KVS_NO_MERGE test in commitmerge
Add additional KVS_NO_MERGE test in kvs/commitmerge.  If KVS
commit-merge=1 module option is set, and all kvs_commit() calls
set the KVS_NO_MERGE flag, the behavior should be similar to when
commit-merge=0.
test/kvs: Add KVS_NO_MERGE test in commit test
Add additional KVS_NO_MERGE test in kvs/commit tests.
test/kvs: Add mixed KVS_NO_MERGE test
Add additional KVS_NO_MERGE tests in kvs/commit test by mixing
both mergeable and non-mergeable commits.

@chu11 chu11 force-pushed the chu11:issue813-kvsnomergeflag branch from fa3d15d to 83706c0 Feb 18, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 18, 2017

Codecov Report

Merging #982 into master will increase coverage by <.01%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master     #982      +/-   ##
==========================================
+ Coverage   75.92%   75.93%   +<.01%     
==========================================
  Files         152      152              
  Lines       25936    25946      +10     
==========================================
+ Hits        19692    19702      +10     
  Misses       6244     6244
Impacted Files Coverage Δ
src/modules/libjsc/jstatctl.c 78.63% <100%> (ø)
src/cmd/flux-kvs.c 76.99% <100%> (ø)
src/modules/resource-hwloc/resource.c 72.88% <100%> (ø)
src/cmd/builtin/hwloc.c 82.63% <100%> (ø)
src/modules/kvs/libkvs.c 75.35% <100%> (ø)
src/modules/live/live.c 51.34% <100%> (ø)
src/bindings/lua/flux-lua.c 81.1% <100%> (ø)
src/modules/aggregator/aggregator.c 76.34% <100%> (ø)
src/bindings/lua/kvs-lua.c 77.3% <100%> (ø)
src/modules/kvs/kvs.c 80.65% <100%> (+0.06%)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9074ac6...83706c0. Read the comment docs.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 18, 2017

Round 2 of this PR. Didn't take as long to cleanup as I anticipated.

Basically it's what I did before, but cleaned up w/ appropriate tinier commits. Instead of using a boolean in the wire protocol to indicate "no merge", I put in an int flag. The int flag got carried into appropriate data structures.

Didn't support "flags" in lua. Noticed flags weren't supported in flux_open() in LUA, so I punted. My LUA knowledge is weak, so part of it was also "i don't have example to copy" :P

I'm not a fan of the flag name "KVS_NO_MERGE", but couldn't really think of anything better. Any better ideas?

I didn't add the ability to "no merge" w/ the flux-kvs command. Think it's necessary?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 18, 2017

Coverage Status

Coverage increased (+0.009%) to 76.209% when pulling 83706c0 on chu11:issue813-kvsnomergeflag into 9074ac6 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 18, 2017

This looks great @chu11! Nice work.

I don't really have a better suggestion on the flag name.

Also, I think it's probably fine to leave the flag off the lua bindings for now. We still have work to do on the KVS API and @grondo has been holding off until that settles down to move the lua bindings forward.

I gather you covered the test @grondo suggested with your "mixed" mode test above?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 21, 2017

@garlick I didn't add that specific test, as I felt it was more or less handled by the last test case I added in:

83706c0

In this one I add a special "modulo" option to the kvs/commit test where each thread (which each have their own flux handle) sends a mixture of commits that set & don't set KVS_NO_MERGE. We can definitely add the test, as it'd just be a variant where some threads always set KVS_NO_MERGE and some never set it.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 21, 2017

@chu11: that test seems great to me!

Also, sorry about the state of the lua binding!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 21, 2017

Thanks Al! Merging.

@garlick garlick merged commit d42f2ca into flux-framework:master Feb 21, 2017

4 checks passed

codecov/patch 96.55% of diff hit (target 75.92%)
Details
codecov/project 75.93% (+<.01%) compared to 9074ac6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 76.209%
Details

@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.