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: store data pointed by valref raw and unencoded in content store #1214

Merged
merged 9 commits into from Sep 28, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Sep 28, 2017

This series of patches updates the KVS so that treeobj valref objects will read/write data to the content store in raw/unencoded form.

The key work was to ensure the internal KVS cache could handle storing raw data instead of only json. After this work, the remaining work sort of "fell out" of this. On lookup, create cache entries that contain raw data and handle them appropriately for valref objects. On commits, cache entries with raw data will be written out as raw data (instead of everything being assumed json).

chu11 added some commits Sep 22, 2017

modules/kvs/cache: Refactor cache_entry_create
Convert cache_entry_create() to not accept any parameters, it now
only creates an empty cache entry.

Create new function cache_entry_create_json(), that takes an optional
json object to hold the cache entry data.  If the user passes in
NULL, it behaves identically to calling cache_entry_create().

Adjust callers of cache_entry_create() appropriately throughout.

Add unit tests and coverage appropriately.
modules/kvs/cache: Refactor internal structure
Change internal json_t *o field to void *data, so the internally
cached data is not type specific.
modules/kvs/cache: Add raw cache entry support
Add support for cache entries that support storing raw data.

Included in this patch is new function cache_entry_create_raw(),
cache_entry_is_type_raw(), cache_entry_get_raw(), and
cache_entry_set_raw().

Add unit tests appropriately.
modules/kvs/cache: Support cache data type
Support type field in cache entries, so different types of data
can be stored in the cache entry.

Add new functions cache_entry_type() and cache_entry_is_type_json()
to retrieve/test type.

Add unit tests appropriately.
modules/kvs/lookup: Update for raw opaque data
Update internal lookup API to handle valref references pointing to
raw unencoded data in the content store.

Most notably, missing references have an additional boolean indicating
if data is raw, so that on data load it can be handled appropriately.

In addition, when returning data pointed to by a valref, add checks
to ensure the data is raw in the KVS cache.

Adjust lookup unit tests appropriately.
modules/kvs/commit: Update for raw opaque data
Update internal commit API to handle valref references pointing to
raw unencoded data in the content store.

Most notably, create cache entries with raw data when appropriate,
so it will ultimately get flushed to the content store as raw
unencoded data.
modules/kvs: Update main KVS module for raw data
Update lookup, commit, and KVS cache calls to handle potential for
data being raw or json.  The major change being valref treeobj objects
pointing to raw unencoded data in the content store.

Fixes #1187
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 28, 2017

Codecov Report

Merging #1214 into master will decrease coverage by <.01%.
The diff coverage is 82.58%.

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
- Coverage   78.23%   78.22%   -0.01%     
==========================================
  Files         158      158              
  Lines       29142    29283     +141     
==========================================
+ Hits        22800    22908     +108     
- Misses       6342     6375      +33
Impacted Files Coverage Δ
src/common/libkvs/treeobj.c 82.85% <100%> (-0.56%) ⬇️
src/modules/kvs/commit.c 76.68% <60%> (-1.16%) ⬇️
src/modules/kvs/kvs.c 64% <71.11%> (-0.11%) ⬇️
src/modules/kvs/cache.c 91.84% <92.94%> (+0.61%) ⬆️
src/modules/kvs/lookup.c 83.88% <94.28%> (-1.16%) ⬇️
src/common/libflux/content.c 86.66% <0%> (-3.34%) ⬇️
src/common/libutil/dirwalk.c 93.57% <0%> (-0.72%) ⬇️
src/common/libflux/future.c 88.31% <0%> (-0.47%) ⬇️
... and 8 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 28, 2017

Coverage Status

Coverage increased (+0.003%) to 78.642% when pulling 9428d33 on chu11:issue1187-7 into 92a797f on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Sep 28, 2017

just realized I didn't do one specific test. I should add some tests to try and handle the specific case of putting content into the content store via flux content store and read it back via the flux kvs command (and put a valref object into the kvs along the way).

{
struct cache_entry *hp;

if ((data && len <= 0) || (!data && len)) {

This comment has been minimized.

@garlick

garlick Sep 28, 2017

Member

The content cache allows a size = 0 blob to be stored. Probably we should allow it in the KVS and add test coverage. Not sure if it worked before, but this would prevent it I guess.

I'll review more closely tomorrow - just noticed that on a quick skim.

This comment has been minimized.

@chu11

chu11 Sep 28, 2017

Author Contributor

the intent was if len == 0, data should == NULL. Would this be ok?

This comment has been minimized.

@garlick

garlick Sep 28, 2017

Member

Oh duh, sorry, I guess I didn't look closely enough there!

This comment has been minimized.

@garlick

garlick Sep 28, 2017

Member

I'll open a separate bug on ability to store zero length data under a key as there do seem to be some issues there, unrelated to this PR.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 28, 2017

OK, I opened #1215 on zero length values as a test coverage issue, since it seems to work OK actually.

I made a pass through the code and didn't spot anything wrong. Since the test coverage is pretty good, I'd be inclined to give the thumbs up, though I think you're proposed test addition is a good one to add before we merge.

I do apologize that you had to do so much work to accomplish a seemingly simple change. As we discussed earlier, I think the ad-hoc, cache-centric mechanism used in the KVS to implement the reactive idiom is at fault. We most likely don't benefit from caching raw values (as opposed to decoded JSON metadata), but we're forced into it because that's the way we stall/restart message handlers while we wait for content RPC's. I'll try to get a bug open on that work so we can track it in the plan and decide if it's worth doing sooner rather than later. Had we done it before this, I think you would have had an easier time.

test/kvs: Add kvs raw unit tests
Add tests to ensure data written to content store in raw form
via the kvs can be read outside of the KVS.  Also verify that
valref tree objects can be put into the KVS to point to data
already in the content store.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 28, 2017

Coverage Status

Coverage decreased (-0.004%) to 78.634% when pulling 3e3c6ed on chu11:issue1187-7 into 92a797f on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Sep 28, 2017

restarted some builds, write error one and a #1169 one.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 28, 2017

OK, thanks @chu11 this is progress!

@garlick garlick merged commit 19d06f1 into flux-framework:master Sep 28, 2017

4 checks passed

codecov/patch 82.58% of diff hit (target 78.23%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +4.34% compared to 92a797f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 78.634%
Details

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.