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: Return error that occurs in kvs.relayfence #1309

Merged

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Dec 15, 2017

Note that this PR comes after #1308 (patches in #1308 are included here). I will rebase after #1308 is merged in.

I originally had this patch mixed in with my remove namespace PR, but it seems independent of everything else, so I've now spliced it into its own PR.

The relayfence_request_cb() function presently does not return an error to the original rpc fence/commit request if an error occurs within this function. An error could be returned to the user only within the commit_apply() path of the code if we reached that path and an error occurred within there.

This patch fixes the problem by effectively calling the error path from commit_apply() if an error occurs within this function.

Unfortunately, outside of some serious code instrumentation, this patch is basically untestable and I bet will have about 0% diff coverage. The error paths are either "impossible" paths or extreme racey paths.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 15, 2017

Coverage Status

Coverage decreased (-0.05%) to 78.518% when pulling bc1f61b on chu11:relayfencereturnerror-pushbranch into 2879f60 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 15, 2017

Codecov Report

Merging #1309 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1309      +/-   ##
==========================================
+ Coverage   78.22%   78.23%   +0.01%     
==========================================
  Files         154      154              
  Lines       27941    27954      +13     
==========================================
+ Hits        21857    21871      +14     
+ Misses       6084     6083       -1
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 64.9% <0%> (-0.67%) ⬇️
src/broker/overlay.c 74.2% <0%> (+0.31%) ⬆️
src/common/libflux/message.c 81.6% <0%> (+0.35%) ⬆️
src/common/libflux/request.c 89.74% <0%> (+1.28%) ⬆️
src/modules/connector-local/local.c 74.38% <0%> (+1.43%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2017

While driving home, it occurred to me that I implemented this backwards.

The original error_event_send() handles an array of names. I wrote a new function error_event_send_to_name(), which converts a single name to an array with one name in it so it can call error_event_send(). I shouldn't have done that. I should convert the original function to process only a single name and then write a function that handles an array. That function would then call the singular name function.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2017

Ugh ... never mind that prior comment. I was confusing error_event_send() with error_event_cb(). What I did is the right thing.

@chu11 chu11 force-pushed the chu11:relayfencereturnerror-pushbranch branch from bc1f61b to 1e9dbd3 Dec 15, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2017

rebased on master and pushed. Expecting near 0% diff coverage.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 15, 2017

Coverage Status

Coverage decreased (-0.07%) to 78.516% when pulling 1e9dbd3 on chu11:relayfencereturnerror-pushbranch into fb37531 on flux-framework:master.

@chu11 chu11 referenced this pull request Dec 15, 2017

Merged

kvs: add more kvs stats #1310

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 15, 2017

Just checking: does this new error path ensure that at most one event is generated per commit?

It does seem like you could shorten error_event_send_to_name() quite a bit by using json_pack() to create the single element array...

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2017

Yes. Basically if we reach commit_mgr_process_fence_request() then the fence is now a "ready commit" and enters the codepath that would return an error via commit_apply() -> error_event_send(). This captures errors in which commit_mgr_process_fence_request() fails or is never reached, so an error event is only generated once.

Good point, json_pack() would be a lot better. I'll cleanup, rebase, and re-push.

modules/kvs: Fix relayfence corner case
In the event an error occurs within relayfence there was no way to
return an error to the original client requests.  Return an error
via the error_event_send() path similar to how an error can occur
during a commit error.

@chu11 chu11 force-pushed the chu11:relayfencereturnerror-pushbranch branch from 1e9dbd3 to 0879a26 Dec 15, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 15, 2017

re-based and re-pushed

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 15, 2017

Coverage Status

Coverage decreased (-0.01%) to 78.566% when pulling 0879a26 on chu11:relayfencereturnerror-pushbranch into 72d3059 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 16, 2017

Great thanks!

@garlick garlick merged commit e704a7f into flux-framework:master Dec 16, 2017

4 of 5 checks passed

codecov/patch 0% of diff hit (target 78.22%)
Details
buildbot/core_standard Build done.
Details
codecov/project 78.23% (+0.01%) compared to 6cdb269
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 78.566%
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.