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: commit/fence return root ref / sequence after transaction #2018

Merged
merged 8 commits into from Feb 15, 2019

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Feb 14, 2019

After a kvs commit/fence, have the rpc return the root reference / seq of the resulting root that has all of the changes in a transaction. This root ref / seq can then be used for causal consistency. Add associated library "get" functions in libkvs/kvs_commit.

The more questionable part of this PR is probably all of the new options I added in flux-kvs. Now every write operation (put, mkdir, link, unlink) can output the resulting treeobj, root ref, or sequence number after the write operation has completed. Is it too much? If it's too much, I'd have to add tests another way.

@garlick
Copy link
Member

garlick commented Feb 14, 2019

It's useful to get something back that can be fed into flux kvs get --at (treeobj) or flux kvs wait (version), but I'm not sure if the blobref option has much practical value?

I think I added that to getroot but now I'm not sure why?

@chu11
Copy link
Member Author

chu11 commented Feb 14, 2019

I think I added that to getroot but now I'm not sure why?

Yeah, I just sort of copied it from getroot. You probably added it just to add coverage to flux_kvs_getroot_blobref() and similar functions?

@chu11
Copy link
Member Author

chu11 commented Feb 14, 2019

hmmm a lot of causal consistency errors, wonder if I messed up the test in some way.

FAIL: t1001-kvs-internals.t 43 - kvs: causal consistency (mkdir)
     t1001-kvs-internals.t:  FAIL: N=53  PASS=51  FAIL=1 SKIP=1 XPASS=0 XFAIL=0

@chu11
Copy link
Member Author

chu11 commented Feb 14, 2019

hmmm a lot of causal consistency errors, wonder if I messed up the test in some way.

I guess I was using the sh -- option incorrectly or the version on travis doesn't like the option's use. I don't need to run these particular tests under a sh call, so just tweaked the tests a bit.

Rebased and repushed.

@chu11 chu11 force-pushed the issue1994 branch 2 times, most recently from a8612c0 to b069811 Compare February 15, 2019 00:38
@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

just thought of a way to enhance the causal consistency tests better by using the pause/unpause setroots event, repushed

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop the blobref accessor in the API and the flux-kvs subcommands, since it's likely not helpful.

I feel less strongly about this, but the flux_kvs_fence_get_*() aliases don't seem all that helpful. Could we drop them and just document that the commit accessors work for fences too?

@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

Let's drop the blobref accessor in the API and the flux-kvs subcommands, since it's likely not helpful.

including getroot?

I feel less strongly about this, but the flux_kvs_fence_get_*() aliases don't seem all that helpful.

sounds like a plan. I actually just realized I didn't add manpages for these functions, so gotta add that too.

@garlick
Copy link
Member

garlick commented Feb 15, 2019

including getroot?

OK by me. I don't think it has any users and I don't exactly know what I was thinking.

@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

re-pushed with suggested changes

@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

re-push, had a chain-lint failure

Have responses to a kvs commit/fence request return the root reference
and sequence that the transaction was committed to.
@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

rebased / re-pushed

@garlick
Copy link
Member

garlick commented Feb 15, 2019

The blobref accessors in the API are still there. Is there a use for those?

@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

The blobref accessors in the API are still there. Is there a use for those?

nah, I left it b/c "it's in getroot too". I can axe it.

Add convenience get functions for retrieving treeobj
or sequence from commit & fence responses.

Fixes flux-framework#1994
Support a new -O and -s option on write operations like put,
unlink, link, and mkdir.  The option will output the RFC11 root
treeobj or root sequence number upon a successful write.
These outputs can be used for causal consistency, such as by feeding
the treeobj to a lookup-at or the sequence number to a wait version.
@chu11
Copy link
Member Author

chu11 commented Feb 15, 2019

re-pushed with removal of flux_kvs_commit_get_blobref().

@codecov-io
Copy link

Codecov Report

Merging #2018 into master will decrease coverage by 0.02%.
The diff coverage is 80.48%.

@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
- Coverage   80.58%   80.56%   -0.03%     
==========================================
  Files         179      179              
  Lines       28779    28845      +66     
==========================================
+ Hits        23191    23238      +47     
- Misses       5588     5607      +19
Impacted Files Coverage Δ
src/common/libkvs/kvs_getroot.c 86.88% <100%> (+0.21%) ⬆️
src/modules/kvs/kvs.c 65.48% <75%> (ø) ⬆️
src/common/libkvs/kvs_commit.c 77.46% <77.35%> (-2.54%) ⬇️
src/cmd/flux-kvs.c 82.2% <90%> (-0.07%) ⬇️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/common/libflux/message.c 81.39% <0%> (-0.25%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.24%) ⬇️
src/broker/modservice.c 80.76% <0%> (+1.92%) ⬆️

@garlick
Copy link
Member

garlick commented Feb 15, 2019

Thanks!

@garlick garlick merged commit 0f4ad9a into flux-framework:master Feb 15, 2019
@chu11 chu11 deleted the issue1994 branch June 5, 2021 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants