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: Rename internal "commit" API to "kvstxn" API #1348

Merged
merged 12 commits into from Mar 5, 2018

Conversation

4 participants
@chu11
Copy link
Contributor

chu11 commented Mar 1, 2018

Per discussion in #1344. I originally was going to do a lot more work before sending in a PR, but there were more "mini cleanups" to do a long the way than I realized, so I think it's a good stopping point for a PR. Per discussion in the issue, this PR handles the first bullet point I listed.

the "internal commit API" should be renamed to "transactions".

I chose to name the api kvstxn, as I wanted to differentiate this as not the normal "txn" API, as special things like merges and what not can occur.

chu11 added some commits Feb 28, 2018

modules/kvs: Rename "commit" data structure
For consistentcy with other portions of code in Flux,
rename the internal "commit" API to avoid confusion with an
actual "commit" RPC.  Rename it to the "kvstxn" API.

No logical code changes occurred in this commit.  Search and
replacing "commit" to "kvstxn" was mostly done.  Some variables
were renamed as a result (e.g. "cm" with "ktm").  Some
functions were also renamed as a result for clarity (e.g. instead
of "txn", used "transaction" in some locations).

Minor comment adjustments as well.
modules/kvs: Rename #readycommits stat
For consistency in terminology, rename the #readycommits statistic
to #readytransactions.
modules/kvs: Rename commit-merge option
For consistency in terminology, rename commit-merge module
option to transaction-merge.

Update unit tests appropriately.
t/kvs: Rename commitmerge test
For consistency in terminology, rename test to transactionmerge
test.
modules/kvs: Rename function names for consistency
For consistency in terminology, rename several functions with
"commit" in the name to "transaction".

@chu11 chu11 self-assigned this Mar 1, 2018

@chu11 chu11 added this to To do in multi-user parallel jobs via automation Mar 1, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage decreased (-0.02%) to 78.84% when pulling 2cc5b1c on chu11:issue1344-part2 into 5192096 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 2, 2018

Since this hasn't been merged yet, I went ahead and pushed my changes to the "fence" data structure. This handles bullet #2.

the "internal fence API" should be renamed to something TBD

The TBD I chose was "treq", standing for transaction request. I chose this instead of "txnreq" b/c with the "kvstxn" API, I thought have the word "txn" all over the place was a bit confusing.

Just like renaming the "commit" API, there were all these side cleanups I had to do as well.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 2, 2018

Codecov Report

Merging #1348 into master will decrease coverage by 0.01%.
The diff coverage is 86.61%.

@@            Coverage Diff             @@
##           master    #1348      +/-   ##
==========================================
- Coverage   78.54%   78.52%   -0.02%     
==========================================
  Files         162      162              
  Lines       29618    29619       +1     
==========================================
- Hits        23264    23259       -5     
- Misses       6354     6360       +6
Impacted Files Coverage Δ
src/modules/kvs/kvsroot.c 68.31% <75%> (ø) ⬆️
src/modules/kvs/kvs.c 65.65% <77.52%> (+0.2%) ⬆️
src/modules/kvs/kvstxn.c 78.02% <86.3%> (ø)
src/modules/kvs/treq.c 88.59% <98.75%> (ø)
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/connectors/local/local.c 87.4% <0%> (-0.75%) ⬇️
src/common/libflux/msg_handler.c 86.64% <0%> (-0.73%) ⬇️
src/common/libflux/reactor.c 93.44% <0%> (-0.29%) ⬇️
src/common/libflux/message.c 81.01% <0%> (-0.12%) ⬇️
... and 2 more

@chu11 chu11 force-pushed the chu11:issue1344-part2 branch from 4e81923 to 47ca2eb Mar 2, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 2, 2018

Oops, noticed one place I forgot to rename a variable. Went ahead and squashed it into the PR & repushed.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 2, 2018

restarted a builder, hit #731

chu11 added some commits Mar 1, 2018

modules/kvs: Rename "fence" data structure
For consistentcy with other portions of code in Flux,
rename the internal "fence" API to avoid confusion with an
actual "fence" RPC.  Rename it to the "treq" API.

No logical code changes occurred in this commit.  Search and
replacing "fence" to "treq" was mostly done.  Some variables
were renamed as a result (e.g. "fm" with "trm").  Some
functions were also renamed as a result for clarity (e.g. instead
of "fences", used "transactions" in some locations).

Minor comment adjustments as well.
modules/kvs: Rename #fences stat
For consistency in terminology, rename the #fences statistic
to #transactions.

Update a unit test appropriately.
modules/kvs: Rename treq_get_json_ops()
Shorten to treq_get_ops(), the json is superfluous.
modules/kvs: Update use of word "fence"
For consistency in terminology, update use of the word "fence"
throughout code.  Update functions, variables, log messages,
and strings with the name in it to use "transaction" or an
appropriate term.

@chu11 chu11 force-pushed the chu11:issue1344-part2 branch from 47ca2eb to 2cc5b1c Mar 3, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 3, 2018

Oops, noticed one other place I forgot to rename a variable. Already squashed & repushed.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 5, 2018

Looks straightforward. I'll go ahead and merge. Thanks!

@garlick garlick merged commit 3e73946 into flux-framework:master Mar 5, 2018

4 checks passed

codecov/patch 86.61% of diff hit (target 78.54%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +8.06% compared to 5192096
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.84%
Details

multi-user parallel jobs automation moved this from To do to Done Mar 5, 2018

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.