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 libkvs kvs-classic #2017

Merged
merged 6 commits into from Feb 15, 2019

Conversation

@chu11
Copy link
Contributor

commented Feb 14, 2019

One interesting change to note of. The old flux_kvs_get_dir() would convert an empty string key to the root directory (".") key b/c it was a compatibility thing with the python kvs code. I just changed the default key in python from "" to ".", which I think should be fine. It doesn't make sense to me that the default key would be an empty string. I assume some long ago legacy behavior?

@chu11 chu11 force-pushed the chu11:remove_libkvsclassic branch from c9f8917 to 923d7cf Feb 14, 2019
@grondo

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

I just changed the default key in python from "" to ".", which I think should be fine. It doesn't make sense to me that the default key would be an empty string. I assume some long ago legacy behavior?

Yes, IIRC early on when we thought kvsdir_t was a nice interface it made sense that the default "dir" was the root directory, ".". Probably fine to require users or bindings to use "." now

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Yes, IIRC early on when we thought kvsdir_t was a nice interface it made sense that the default "dir" was the root directory, ".". Probably fine to require users or bindings to use "." now

The legacy behavior I was referring to the was fact the default key was an empty string. I changed it so the default key is now ".". Are you thinking we not have a default at all?

@grondo

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Sorry, I meant the default for flux_kvsdir_get() or what not (I think the C api call used to return the root dir if you passed an empty string? I could be mistaken though)

I think what you did is correct.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Only travis error appears to be #2015, so gotta fix that first.

chu11 added 6 commits Feb 13, 2019
The default key of kvs.get_dir() was the empty string "".  The internal
C function flux_kvs_get_dir() would convert the empty string into
the key ".".  Instead, make the default the ".".
Remove references to "classic" API.
@chu11 chu11 force-pushed the chu11:remove_libkvsclassic branch from 923d7cf to a241199 Feb 14, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

rebased

@chu11 chu11 changed the title kvs: Remove libkvsclassic kvs: Remove libkvs kvs-classic Feb 14, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Feb 15, 2019

Codecov Report

Merging #2017 into master will increase coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2017      +/-   ##
==========================================
+ Coverage   80.34%   80.55%   +0.21%     
==========================================
  Files         179      178       -1     
  Lines       28898    28727     -171     
==========================================
- Hits        23217    23140      -77     
+ Misses       5681     5587      -94
Impacted Files Coverage Δ
src/common/libflux/response.c 81.48% <0%> (-1.24%) ⬇️
src/common/libflux/message.c 81.27% <0%> (-0.25%) ⬇️
src/broker/module.c 82.74% <0%> (+0.26%) ⬆️
src/broker/modservice.c 79.8% <0%> (+0.96%) ⬆️
src/common/libflux/mrpc.c 88.93% <0%> (+1.18%) ⬆️
src/modules/barrier/barrier.c 78.62% <0%> (+2.06%) ⬆️
Copy link
Member

left a comment

LGTM!

@garlick garlick merged commit bb9ef77 into flux-framework:master Feb 15, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch Coverage not affected when comparing dc1fcf7...a241199
Details
codecov/project 80.55% (+0.21%) compared to dc1fcf7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.