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

modules/kvs: Fix potential to exceed nprocs #1304

Merged
merged 1 commit into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Dec 12, 2017

Fix corner case in which nprocs could be exceeded and additional
operations appended onto a fence. Now, user will receive an EOVERFLOW
error if they have exceeded the nprocs count.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.02%) to 78.566% when pulling d851e43 on chu11:fencecornercase into 53697fa on flux-framework:master.

modules/kvs: Fix potential to exceed nprocs
Fix corner case in which nprocs could be exceeded and additional
operations appended onto a fence.  Now, user will receive an EOVERFLOW
error if they have exceeded the nprocs count.

Document EOVERFLOW in documentation.

@chu11 chu11 force-pushed the chu11:fencecornercase branch from d851e43 to 5b587ad Dec 12, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 12, 2017

oops, forgot to add new word for spell check. Re-push.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 12, 2017

Codecov Report

Merging #1304 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1304      +/-   ##
==========================================
+ Coverage   78.22%   78.24%   +0.02%     
==========================================
  Files         154      154              
  Lines       27931    27935       +4     
==========================================
+ Hits        21848    21859      +11     
+ Misses       6083     6076       -7
Impacted Files Coverage Δ
src/modules/kvs/fence.c 83.33% <100%> (+0.83%) ⬆️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
src/common/libflux/message.c 81.01% <0%> (-0.59%) ⬇️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
src/common/libflux/future.c 89.25% <0%> (+0.46%) ⬆️
src/common/libutil/dirwalk.c 94.28% <0%> (+0.71%) ⬆️
src/common/libflux/mrpc.c 86.66% <0%> (+1.17%) ⬆️
src/modules/connector-local/local.c 74.59% <0%> (+1.63%) ⬆️
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.03%) to 78.577% when pulling 5b587ad on chu11:fencecornercase into 53697fa on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 13, 2017

In "nature", this can't occur because the completed fence is removed from the hash upon reaching the count, so the next request arriving with the same name is like a brand new one. Correct?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 13, 2017

At the moment this is not the case, all fences stay on the hash until the fence/commit is done (may it be done by success or error). I think this is what is desired. Because fences are identified by name, we don't want two fences with the same name lingering around.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 13, 2017

Oh, I see - it happens when the count has been reached but the commit is still not applied. OK, good catch then!

@garlick garlick merged commit b43aca0 into flux-framework:master Dec 13, 2017

5 checks passed

buildbot/core_standard Build done.
Details
codecov/patch 100% of diff hit (target 78.22%)
Details
codecov/project 78.24% (+0.02%) compared to 53697fa
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 78.577%
Details

@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.