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: remove unused internal transaction request API code #6595

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Feb 1, 2025

With the removal of KVS fence support in #6592, a large amount of the transaction request ("treq") API is no longer needed. Remove that code and code that uses it.

As a side note, you may notice the "treq" API is now very simple. Here's the internal structs

struct treq_mgr {
    zhash_t *transactions;
};

struct treq {
    char *name;
    const flux_msg_t *request;
    int flags;
};

So I hope I can "squash" this into the kvsroot API, but that is for a follow up PR. This PR is for "obvious" removal of dead code.

chu11 added 9 commits January 31, 2025 11:08
Problem: There is an errant typo in which an upper case example
is written lower case.

Make the example character upper case.
Problem: With KVS fences now removed, transaction request code no
longer has to deal with counting the number of processes associated
with each transaction (i.e. has the transaction barrier been reached).

Solution: Remove the functions treq_count_reached() and
treq_get_nprocs().  Update treq_create() and treq_create_rank() to
no longer take a "nprocs" parameter.  Remove all internal code related
to "counts" or "nprocs".

Update callers and unit tests accordingly.
Problem: The treq_create() function is no longer used by the KVS.

Solution: Remove the function.  Rename the treq_create_rank() function
to treq_create() as it is the only "create" function now.  Collapse
some code as necessary.  Update tests accordingly.
Problem: The treq API includes a function treq_add_request_copy().
This function was initially created to support fences, since fences
would have multiple RPC requests attached to it.  Now that fences
are no longer supported, this function is no longer needed.

Solution: Remove treq_add_request_copy() and have a request copy
be added to a transaction via treq_create().  Internally, the
requests list can be removed and a single request copy can be saved
in a transaction.  Adjust callers and tests accordingly.
Problem: The treq_get_ops() and treq_add_request_ops() functions
are no longer used.  They were primarily used by fence operations, which
are no longer supported in the KVS.

Remove them, remove internal storage of ops, and remove associated unit tests.
Problem: The treq_iter_request_copies() function iterates through
all messages internally copied into a transaction request.  However,
since KVS fences have been removed, the maximum number of requests
can be stored is now just 1.  So having a function that iterates
over the requests is excessive.

Solution: Remove treq_iter_request_copies() and replace with a new
functiontreq_get_request(), which just returns the stored request
message.  Adjust all callers.  Adjust tests and remove now unnecessary
tests.
Problem: When a kvs root is removed, it cleans up incomplete fences.
This code is no longer needed now that KVS fence support has been removed.

Solution: Remove code that cleans up incomplete fences.
Problem: The function treq_mgr_iter_transactions() is no longer
used.

Solution: Remove it and all associated tests.
Problem: The treq processed flag set via treq_mark_processed()
and retrieved via treq_get_processed() is no longer needed now that
fence support is removed.

Remove the functions, their use, and associated tests.
Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.51%. Comparing base (ace8d8a) to head (0d97cc2).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/modules/kvs/kvs.c 57.14% 3 Missing ⚠️
src/modules/kvs/treq.c 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6595      +/-   ##
==========================================
+ Coverage   79.46%   79.51%   +0.04%     
==========================================
  Files         531      531              
  Lines       88202    88090     -112     
==========================================
- Hits        70091    70046      -45     
+ Misses      18111    18044      -67     
Files with missing lines Coverage Δ
src/common/libkvs/kvs_txn_compact.c 63.35% <ø> (ø)
src/modules/kvs/treq.c 63.07% <80.00%> (+18.56%) ⬆️
src/modules/kvs/kvs.c 72.79% <57.14%> (+0.44%) ⬆️

... and 6 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant