kvs: move "wait_version" API into "kvsroot" API #6628
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: In kvsroot and kvs_wait_version tests we should bail out if a call to cache_create() fails. This is missed in some cases. Call BAIL_OUT if cache_create() fails.
Problem: The kvsroot convenience tests are scattered around in one function. Split out convenience function tests into new functions.
Problem: Some test functions in test/kvsroot.c are poorly name. Add more descriptive function names.
Problem: A count is initialized to zero but the variable is never used. Remove the dead line of code.
Problem: Some unit tests in test/kvs_wait_version.c use a global variable for counting. Instead, a counter should be passed as an argument to the callback. Remove the global counting variable. Pass a local variable as an argument to a callback instead.
Problem: The internal function kvsroot_destroy() does not preserve errno. Callers must do it themselves. This is not the common code pattern in flux-core. Have kvsroot_destroy() preserve errno. Remove "saving errno" that callers are doing.
Problem: The internal function kvs_wait_version_destroy() does not preserve errno when it is called. Preserve errno when kvs_wait_version_destroy() is called.
Problem: In the kvsroot data structure, a zhash_t is used for transaction requests. A zhashx_t is preferred in flux-core nowadays because it does not have as large of a initial memory footprint. Convert transaction_requests from a zhash_t to zhashx_t.
Problem: The primary roothash is a zhash_t, however it is more common to use a zhashx_t in flux-core since the initial memory footprint is much smaller. Convert roothash to use a zhashx_t over a zhash_t.
Problem: The kvs wait_version API is mostly a set of convenience functions on the kvsroot. The API is quite small and exists as a separate file mostly due to the legacy implementation of KVS "watchers" in the KVS. Move the wait_version API functions into kvsroot, which ends up only being 3 new convenience functions. Move over the kvs wait_version unit tests into the kvsroot ones. Remove the old wait_version API files and test files.
Problem: The wait_version_list is a zlist_t, which does not allow users to remove items from the list while iterating through the list. A zlistx_t would allow removal of items while iterating through the list. Solution: Convert wait_version_list to a zlistx_t. In kvs_wait_version_remove_msg() adjust code to no longer need a temporary list of messages to be removed.
Problem: In the kvsroot code a removelist is a zlist_t. It is the only zlist or zhash that has not been converted into a "x" equivalent in the kvsroot. For consistency, update removelist from zlist_t to zlistx_t.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6628 +/- ##
=======================================
Coverage 83.86% 83.86%
=======================================
Files 534 533 -1
Lines 88358 88340 -18
=======================================
- Hits 74100 74086 -14
+ Misses 14258 14254 -4
|
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.
Built on top of #6627
The "wait_version" convenience functions sit in their own file / API mostly due to legacy of their original implementation from old KVS watchers. It's only 3 convenience functions operating on kvsroot.
Move them and tests into the "kvsroot" API, therefore removing the kvs_wait_version files and tests.