content: track RFC 10 protocol changes #4299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: some content backing store tests refer to "rooref" when "rootref" was intended. This appears to have proliferated via cut and paste. Fix typos.
Problem: the size of the hash digest corresponding to a blobref is not disclosed to users of the blobref class. Have blobref_validate_hashtype() return the digest size on success, instead of zero. Update tests.
Problem: now that protocols use hash digests directly, it is inconvenient to have to convert a blobref from blobref_hash() back to a digest. Add blobref_hash_raw() which just computes the digest. Add unit tests.
Problem: libcontent internal functions are named inappropriately given RFC 10 protocol changes in the pipeline. We will still want functions that operate on blobrefs for convenience, but also more efficient ones that match the protocol. In anticipation of this, rename: content_load() -> content_load_byblobref() content_store_get() -> content_store_get_blobref() Update users.
garlick
force-pushed
the
content_optimize
branch
from
2e594e7
to
1b5b7b2
Compare
chu11
approved these changes
Apr 25, 2022
src/common/libcontent/content.c
Outdated
| } | ||
|
|
||
| int content_load_get (flux_future_t *f, const void **buf, int *len) | ||
| { | ||
| return flux_rpc_get_raw (f, buf, len); | ||
| } | ||
|
|
||
|
|
Problem: the RFC 10 content protocol has been updated to use raw hash digests instead of blobrefs, but flux-core still uses blobrefs. Convert to the new protocol. This includes updating - libcontent - broker content cache - all the content backing modules In addition, use the first eight bytes (sizeof (size_t)) of the hash digest directly as the hash key in the content cache, which should already be evenly distributed over its range and more efficient than hash digest -> blobref -> Bernstein_hash (blobref) Update content backing module sharness tests which use the load/store protocol directly.
Problem: the althash s3 test fails because it reuses the bucket from the content-s3 test with an incompatible hash. Append the test name to the s3 bucket to avoid this. Also drop the second s3 test that uses sha1, since that was already covered in content-s3.
Problem: a generic test in the althash sharness test had the S3 prereq. Drop S3 prereq. The test is not S3 related.
garlick
force-pushed
the
content_optimize
branch
from
1b5b7b2
to
36cc523
Compare
grondo
approved these changes
Apr 26, 2022
|
Thanks guys, setting MWP. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the protocol change proposed in flux-framework/rfc#330, in which blobref strings are replaced with the raw hash in content load/store RPCs. The broker content-cache also now uses hashes instead of blobrefs as the zhashx key.
The RPC wrappers in libcontent were updated, so their users including
flux-content,flux-dump,flux-restore, and the KVS module did not really have to change, although some of the libcontent functions were renamed.One thing that superficially worked before but no longer works at all after this change is modifying the hash algorithm across an instance restart. If we want to enable a system instance to migrate from sha1 to sha256, a dump/restore will be required and the content back ends will need to treat an attempt to restart under a different hash as a fatal error. Since we don't yet have TOML support for changing the hash algorithm, I think we don't necessarily need to fix that here.