-
Notifications
You must be signed in to change notification settings - Fork 51
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
chu11
wants to merge
9
commits into
flux-framework:master
Choose a base branch
from
chu11:kvs_refactor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+60
−584
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: 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.
Codecov ReportAttention: Patch coverage is
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
|
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.
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
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.