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

add flux_content API #903

Merged
merged 11 commits into from Nov 18, 2016

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Nov 14, 2016

This pr adds public API functions for manipulating the content store, sufficient to replace all the places where we were crafting content store RPC's by hand.

Along the way, libutil/shastring was converted to a more abstract libutil/blobref interface that supports arbitrary hash functions, sha256 support was added to demonstrate/test this capability, and users that were hardwired to sha1 were updated to use this interface.

Finally, the flux-content store --dry-run option was dropped and a test program that performs hashing of blobs without writing them to the content store was added, as this was not a useful user-facing capability.

@garlick garlick added the review label Nov 14, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-0.02%) to 75.931% when pulling a22da6c on garlick:treeobj_jansson into 87f14cd on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 15, 2016

Current coverage is 72.57% (diff: 91.96%)

Merging #903 into master will increase coverage by 0.20%

@@             master       #903   diff @@
==========================================
  Files           157        159     +2   
  Lines         27029      27159   +130   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19561      19711   +150   
+ Misses         7468       7448    -20   
  Partials          0          0          
Diff Coverage File Path
••••••• 71% src/modules/kvs/kvs.c
••••••• 75% src/cmd/builtin/content.c
•••••••• 85% src/broker/content-cache.c
•••••••• 86% new src/common/libflux/content.c
••••••••• 95% new src/common/libutil/blobref.c
•••••••••• 100% src/modules/kvs/libkvs.c
•••••••••• 100% src/modules/kvs/cache.c
•••••••••• 100% src/modules/kvs/json_dirent.c
•••••••••• 100% src/modules/content-sqlite/content-sqlite.c
•••••••••• 100% new src/common/libutil/sha256.c

Powered by Codecov. Last update c5478da...18932ee

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 15, 2016

er just spotted a bug in the way individual blobrefs are computed when a large blob is split. Fix coming.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage decreased (-0.03%) to 75.921% when pulling 10c9783 on garlick:treeobj_jansson into 87f14cd on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 15, 2016

Cool! Should I be worried that test coverage of the added files treeobj.c and content.c is low? ;-)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 15, 2016

Yeah I will fix that.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from 10c9783 to 30967b1 Nov 15, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 15, 2016

Rebased on current master, fixed a bug in treeobj and fleshed out its unit tests a bit more. Not done, just a checkpoint before lunch

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage increased (+0.06%) to 76.016% when pulling 30967b1 on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from 30967b1 to c0ba806 Nov 15, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 15, 2016

Coverage Status

Coverage increased (+0.2%) to 76.109% when pulling c0ba806 on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 15, 2016

Getting happier with the coverage, now I'll write up a man page for the flux_content interfaces.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 16, 2016

Coverage Status

Coverage increased (+0.2%) to 76.112% when pulling 87c787e on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 16, 2016

@garlick, is more work happening here or is this ready for a review and merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 16, 2016

I have some changes queued up that I'd like to tidy up and push, then it will be ready

into multiple store requests internally, this function will fail with errno
set to EFBIG, and `flux_content_store_get_valref()` should be used to
retrieve the result as a treeref instead instead. This function is
idempotent.

This comment has been minimized.

@grondo

grondo Nov 16, 2016

Contributor

I could be confused, but should "retrieve the result as treeref" here be "retrieve results as valref" instead? I'm basing this solely on the function name "flux_content_store_get_valref" as I don't quite understand the API fully yet.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 16, 2016

No you're right. Thanks for catching that! Let me push my changes as I expanded that man page a bit.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from 87c787e to a9a2ea7 Nov 16, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 16, 2016

Coverage Status

Coverage increased (+0.2%) to 76.106% when pulling a9a2ea7 on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from a9a2ea7 to 4651c42 Nov 16, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 16, 2016

@grondo verbally suggested that the "content store" should be referred to as the "content service" to avoid confusion since "store" is one of the operations performed by the service. (This is not to mention the confusing "store_get" and "load_get", where the content service and rpc idioms are mixed!) Just pushed an update to the man pages to hopefully make this a little better.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 16, 2016

The only thing that is still slightly confusing is the valref v. blobref case in flux_content_store_get. If a valref can refer to one or more blobrefs, would it make sense to have the basic function always return a valref? Otherwise, I guess every call to flux_content_store_get has to check for E2BIG and then call flux_content_store_get_valref? My worry would be that most callers would skip that check and have an unexpected failure the one time out of 1000 that the content store request was too large.

I could be worried over nothing though, and since the main user here is the kvs, I think you would know best...

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 16, 2016

I expected that a caller would want either blobrefs or valrefs. If the former, maxblob would be set to zero, and a blob that is too large for the content service would receive an error. If the latter, maxblob would be set to some value (perhaps from getattr content-blob-size-limit), and the result would always be retrieved as a valref.

Besides the misleading man page, I am thinking I have two abstractions mixed up here. Maybe this interface should simply be for single blobrefs and a different interface should be proposed later that covers all the KVS metadata?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 16, 2016

I expected that a caller would want either blobrefs or valrefs. If the former, maxblob would be set to zero, and a blob that is too large for the content service would receive an error. If the latter, maxblob would be set to some value (perhaps from getattr content-blob-size-limit), and the result would always be retrieved as a valref.

Ah! that makes sense! I had assumed (probably due to lazy manpage reading) that a hard max blob size would be configured in the content service -- i.e. a caller to store may not know beforehand if the store operation would potentially be split. Am I way off?

Maybe this interface should simply be for single blobrefs and a different interface should be proposed later that covers all the KVS metadata?

I am not sure -- I think I'd defer to you here. One short term use case that comes to mind, is a module loading system that you mentioned, which could store or map a module into the content store and send out a valref to the module loading service.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 16, 2016

The content service uses the content-blob-size-limit broker attribute to set its maximum. The caller can get this and do the fragmentation on the client side so that it fits with what the server can handle. The main point of the fragmentation is to avoid problems with really huge messages, head of line blocking and all that, so fragmentation on the client is key.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 16, 2016

Ok, I'm sorry for not doing my homework! I had assumed the interface would not allow flux_content_store with size > content-blob-size-limit, but in this API that is the responsibility of the caller? That should get a mention and/or convenience function in the manpage perhaps

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 16, 2016

No it really does feel like this design is inherently confusing.

Let me see if I can make it less so.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from 4651c42 to 2d1d782 Nov 18, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 18, 2016

I scaled the content API back to directly cover the content service operations, and to not grok "valref" at all. This tidies up the few places where those RPCs were being constructed by hand, and as before, also gets rid of the hardwired sha1 code.

The new API returns a flux_rpc_t so it is no longer necessary to have duplicate functions for flux_rpc_then() and flux_rpc_check() - those just work. API is now:

/* flags */
enum {
    CONTENT_FLAG_CACHE_BYPASS = 1,/* request direct to backing store */
    CONTENT_FLAG_UPSTREAM = 2,    /* make request of upstream TBON peer */
};

/* Send request to load blob by blobref.
 */
flux_rpc_t *flux_content_load (flux_t *h, const char *blobref, int flags);

/* Get result of load request (blob).
 * This blocks until response is received.
 * Storage for 'buf' belongs to 'rpc' and is valid until 'rpc' is destroyed.
 * Returns 0 on success, -1 on failure with errno set.
 */
int flux_content_load_get (flux_rpc_t *rpc, void *buf, int *len);

/* Send request to store blob.
 */
flux_rpc_t *flux_content_store (flux_t *h, const void *buf, int len, int flags);

/* Get result of store request (blobref).
 * Storage for 'blobref' belongs to 'rpc' and is valid until 'rpc' is destroyed.
 * Returns 0 on success, -1 on failure with errno set.
 */
int flux_content_store_get (flux_rpc_t *rpc, const char **blobref);

@garlick garlick force-pushed the garlick:treeobj_jansson branch from 2d1d782 to c3935b3 Nov 18, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.2%) to 76.127% when pulling c3935b3 on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from c3935b3 to 6cc55eb Nov 18, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.2%) to 76.145% when pulling c3935b3 on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.2%) to 76.138% when pulling 6cc55eb on garlick:treeobj_jansson into 55dcaaf on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 18, 2016

Wow, that is really nice now, kudos! I really like the idiom here of reusing flux_rpc_then and flux_rpc_check, with custom flux_*_get. Nice work!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 18, 2016

Thanks, well dropping "treeobj" support made it possible to do in the obvious way. This interface offers no help if you want to store something greater than the configured max blob size, but I think that's probably OK here.

I'll rebase after #909 goes in.

garlick added some commits Jan 12, 2016

libflux/content: add flux_content api functions
Instead of requiring users to craft RPCs directly, add an
abstract interface for the content service, layered on
top of the flux_rpc().
libutil/blobref: create abstract blobref functions
Problem: SHA1 hash is hardwired in various parts of Flux,
but the "blobref" was designed in RFC 10 to accommodate
different hash functions.

Rename libutil/shastring to libutil/blobref, change the API
so that it conforms to the abstract design of RFC10, and
add support for sha256, mainly for testing.

Update users: kvs, content store, sqlite backing store,
and flux-content command line tool.
test/blobref: add unit test for libutil/blobref
Exercise new blobref functions.
test/content: use test program, not --dry-run
Problem: tests t0011-content-cache.t and
t0012-content-sqlite.t depended on flux-content --dry-run
option. This function is now performed by a test program.

@garlick garlick force-pushed the garlick:treeobj_jansson branch from 6cc55eb to 18932ee Nov 18, 2016

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 18, 2016

OK, I've rebased this.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.2%) to 76.17% when pulling 18932ee on garlick:treeobj_jansson into c5478da on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 18, 2016

Great! Can I click the button?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 18, 2016

Please :-)

@grondo grondo merged commit a9ff086 into flux-framework:master Nov 18, 2016

4 checks passed

codecov/patch 91.96% of diff hit (target 72.37%)
Details
codecov/project 72.57% (+0.20%) compared to c5478da
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 76.17%
Details

@grondo grondo removed the review label Nov 18, 2016

@grondo grondo referenced this pull request Nov 28, 2016

Closed

Create 0.6.0 release notes #916

@garlick garlick deleted the garlick:treeobj_jansson branch Sep 6, 2017

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.