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

allow hash type to be configured #1051

Merged
merged 8 commits into from May 2, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented May 1, 2017

Although the content cache was written so the hash type could be configured to use an algorithm other than the default sha1, there are a few obstacles to overcome to make that work.

First is to allow the hash type to be configured on the broker command line, e.g.

$ flux start -o,-Scontent.hash=sha256

Next is to have the KVS pick up the configured hash type and use it, since it pre-generates blobrefs to use as hash keys in its internal cache when content store requests are sent off (they can't wait for the response to get the hash key).

Finally, add a test that makes sure this works.

Along the way I updated the names of all the attributes used by the content store to be namespaced in the more modern way: content.name-of-attr rather than content-name-of-attr, and updated users and docs.

garlick added some commits May 1, 2017

broker/content-cache: modernize attr names
Problem: content cache attributes are not properly namespaced.

Rename content-* to content.*.

Update users:  content-sqlite and content sharness tests.
doc/flux-broker-attributes(7): rename content attrs
Update documentation to reflect content-cache attribute
"namespacing".
libutil/blobref: add blobref_validate_hashtype()
Problem: there is no convenient way to test whether
a requested hash algorithm can be used with the blobref
class, short of attempting to generate a blobref.

Add blobref_validate_hashtype() which accepts a
hash name and returns 0 if valid, -1 if invalid.
test/blobref: cover blobref_validate_hashtype()
Add unit test coverage for blobref_validate_hashtype().
broker/content-cache: allow setattr hash type
Problem: hash algorithm is hardwired to use sha1, although
most of the code is written to allow the hash to be
configurable.

Modify content-cache so that the hash type may be
selected by setting the content.hash attribute on the
broker command line.
modules/kvs: get hash type from broker attr
Problem: kvs hardwires hash type to sha1.

Obtain hash type from broker content.hash attribute.
doc/flux-content(1): rename content attrs
Update documentation to reflect content-cache attribute
"namespacing".
test/content: test content.hash=sha256
Add a sharness test that starts a Flux instance with
content.hash=sha256.  The instance loads content-sqlite,
kvs, and resource-hwloc, so a successful startup actually
isn't a bad test.
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 1, 2017

Coverage Status

Coverage decreased (-0.08%) to 77.935% when pulling 17e2cbe on garlick:config_blobref into f1b9b45 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #1051 into master will decrease coverage by 0.07%.
The diff coverage is 82.85%.

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   77.75%   77.67%   -0.08%     
==========================================
  Files         148      148              
  Lines       25739    25752      +13     
==========================================
- Hits        20014    20004      -10     
- Misses       5725     5748      +23
Impacted Files Coverage Δ
src/common/libutil/blobref.c 96% <100%> (+0.16%) ⬆️
src/modules/kvs/kvs.c 80.6% <33.33%> (-0.57%) ⬇️
src/modules/content-sqlite/content-sqlite.c 54.54% <50%> (ø) ⬆️
src/broker/content-cache.c 73.18% <92%> (-1.31%) ⬇️
src/broker/module.c 82.15% <0%> (-1.42%) ⬇️
src/cmd/flux-event.c 65.55% <0%> (-1.12%) ⬇️
src/common/libflux/rpc.c 91.2% <0%> (-1.1%) ⬇️
src/broker/modservice.c 79.2% <0%> (-1%) ⬇️
src/common/libutil/veb.c 98.31% <0%> (-0.57%) ⬇️
... and 6 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 2, 2017

Nice. Especially nice how little code was required to add this feature.

@grondo grondo merged commit 095a509 into flux-framework:master May 2, 2017

4 checks passed

codecov/patch 82.85% of diff hit (target 77.75%)
Details
codecov/project Absolute coverage decreased by -0.07% but relative coverage increased by +5.09% compared to f1b9b45
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 77.935%
Details

@garlick garlick deleted the garlick:config_blobref branch May 2, 2017

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