kvs: remove internal treq api #6604
Conversation
chu11
force-pushed
the
kvs_refactor2
branch
7 times, most recently
from
cbee6fe to
a1cdb05
Compare
src/modules/kvs/kvs.c
Outdated
| if (asprintf (&name, "transaction_req.%u.%u", ctx->rank, ctx->seq++) < 0) | ||
| goto error; |
There was a problem hiding this comment.
Seems like maybe name could be a small static buffer and this malloc could be avoided?
There was a problem hiding this comment.
oh that's a good idea, since we aren't allocating a treq_t anymore we don't have to allocate it .
src/modules/kvs/kvsroot.c
Outdated
| if (asprintf (&name, "transaction_req.%u.%u", rank, seq) < 0) | ||
| goto error; |
src/modules/kvs/kvsroot.c
Outdated
| if (zhash_insert (root->transaction_requests, | ||
| name, | ||
| (void *)flux_msg_incref (request)) < 0) { | ||
| errno = EEXIST; | ||
| goto error; | ||
| } |
There was a problem hiding this comment.
Just wondering if this is the best container? I think we decided of the hashes, zhashx had more sane growth management than zhash. I guess if the number of messages stored in the hash is always small that may not come into it.
Would a flux_msglist_t possibly be a better fit or is the lookup by name important?
Sorry I'm not entirely remembering how this code works so I'm just asking the question.
There was a problem hiding this comment.
I was thinking of that. But left it as a zhash just because that was what was there before. Could think of that for some potential cleanup / improvement.
I had contemplated a zlist b/c now that we only support commits, the "transactions" should reasonably be done in order. So name matching should always occur with the first entry. But not sure if that's worth it vs a hash since we would want to double check and match names anyways.
Problem: "future" is typoed as "futre" in several places. "content" was typoed as "confent" in one place. Fix the typos.
Problem: Several kvsroot convenience functions do not have invalid input tests. Add tests to kvs/test/kvsroot.c to ensure invalid inputs to kvsroot_setroot() and kvsroot_check_user() are checked.
Problem: In a kvsroot_check_user() test, the cred is not initialized before it is first used. Initialize it to avoid a warning.
Problem: In the function finalize_transaction_req() a parameter is passed in that is never used. Remove the unused parameter.
Problem: The flags stored in a transaction request are never used/accessed. Remove the flags parameter to treq_create(). Remove the function treq_get_flags(). Remove unit tests. Update callers accordingly.
Problem: The treq_t convenience type for storing a transaction request is now a struct of just two values: a message request and a name created for that request. Having a unique struct and an API for it is now excessive. Solution: Remove the treq_t type and API for its creation and access of internal variables. Remove all related unit tests. Change the treq_mgr_add_transaction() function take the message request and a name. Have treq_mgr_lookup_transaction() return a flux_msg_t now. Update tests and users accordingly.
Problem: The treq_mgr_t struct is now just a zhash. So there's no need to use it and the treq-mgr api vs a zhash directly. Convert kvsroot to use a zhash instead of a treq manager. Add convenience function kvsroot_save_transaction_request() with unit tests. Update all callers that previously used treq mgr functions to directly use zhash functions instead.
Problem: The treq API is no longer used. Remove it and related unit tests.
|
re-pushed with the static buffer fix recommended above. It ends up I did have a rebase mistake in my commit history which I also fixed. I'll set MWP |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6604 +/- ##
=======================================
Coverage 79.50% 79.50%
=======================================
Files 531 530 -1
Lines 88363 88300 -63
=======================================
- Hits 70251 70207 -44
+ Misses 18112 18093 -19
|
effectively "squash" the treq api into the kvsroot API since there was almost nothing left in the treq api. Removing a whole sub-library within the kvs. woot!