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: add read-your-writes consistency tests #1907

Merged
merged 6 commits into from Jan 7, 2019

Conversation

@chu11
Copy link
Contributor

commented Jan 4, 2019

per discussion in #1832

@codecov-io

This comment has been minimized.

Copy link

commented Jan 4, 2019

Codecov Report

Merging #1907 into master will decrease coverage by 0.04%.
The diff coverage is 74.62%.

@@            Coverage Diff             @@
##           master    #1907      +/-   ##
==========================================
- Coverage   80.13%   80.09%   -0.05%     
==========================================
  Files         196      196              
  Lines       35065    35124      +59     
==========================================
+ Hits        28101    28134      +33     
- Misses       6964     6990      +26
Impacted Files Coverage Δ
src/modules/kvs/kvsroot.c 71.66% <100%> (+0.48%) ⬆️
src/modules/kvs/kvs.c 66.92% <73.84%> (+0.35%) ⬆️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
src/modules/connector-local/local.c 74.07% <0%> (-0.75%) ⬇️
src/common/libflux/handle.c 83.52% <0%> (-0.47%) ⬇️
src/cmd/flux-module.c 83.49% <0%> (-0.24%) ⬇️
src/common/libflux/message.c 81.27% <0%> (-0.13%) ⬇️
@garlick

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Nice!

We could probably use a prominent block comment on the pause/unpause request handlers that this is a test hook, and maybe a reference to the test code that uses it.

Would this be a bit simpler (and just as effective) if the pause/unpause applied equally to all namespaces, and if there could be only one in effect? (Just a thought - if this is as simple as it gets I'm OK with it!)

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

We could probably use a prominent block comment on the pause/unpause request handlers that this is a test hook, and maybe a reference to the test code that uses it.

Good idea, will add.

Would this be a bit simpler (and just as effective) if the pause/unpause applied equally to all namespaces, and if there could be only one in effect? (Just a thought - if this is as simple as it gets I'm OK with it!)

Hmmm. It probably adds a small bit of complexity as I'd have to iterate through all namespaces on a pause / unpause. Not much, but it's something. Are you imaging a test case that would need that?

chu11 added 6 commits Dec 20, 2018
Support new set of RPCs that will allow user to pause or unpause
the receive of setroot events.  When paused, setroot events will
be queued up and then processed after an unpause.  This functionality
will be useful for testing.

Callers should be wary of usage, as pausing setroot processing on
a particular rank will make all kvs writes hang on that rank until
unpause has been issued.
Parse field for primary namespace, to avoid breakage if other
namespaces exist.
@chu11 chu11 force-pushed the chu11:issue1832 branch from 8da0b39 to b8080cf Jan 7, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

re-pushed with extra comments added

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Hmmm. It probably adds a small bit of complexity as I'd have to iterate through all namespaces on a pause / unpause. Not much, but it's something. Are you imaging a test case that would need that?

No no, I was just thinking maybe it could be done without possibly stalling/restarting when the namespace under test isn't known yet, or adding state to the namespace (as opposed to tracking it separately) and that that might be a bit simpler to implement. This is fine.

@garlick

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

This looks fine, and please disregard my suggestion. I see now that would probably be a mess.

Thanks!

@garlick garlick merged commit 330c702 into flux-framework:master Jan 7, 2019
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 74.62% of diff hit (target 80.13%)
Details
codecov/project 80.09% (-0.05%) compared to 0bcf910
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
3 participants
You can’t perform that action at this time.