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

wrexecd: move sending of wreck.state.complete event after job archival #955

Merged
merged 1 commit into from Jan 18, 2017

Conversation

Projects
None yet
5 participants
@SteVwonder
Copy link
Member

SteVwonder commented Jan 16, 2017

Closes #728

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 16, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.332% when pulling 870ffed on SteVwonder:move-complete-event into 15709a9 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 16, 2017

Current coverage is 75.91% (diff: 87.50%)

Merging #955 into master will decrease coverage by <.01%

@@             master       #955   diff @@
==========================================
  Files           153        153          
  Lines         26028      26030     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19760      19761     +1   
- Misses         6268       6269     +1   
  Partials          0          0          
Diff Coverage File Path
•••••••• 87% src/modules/wreck/wrexecd.c

Powered by Codecov. Last update 3e05f58...81342b9

@SteVwonder SteVwonder force-pushed the SteVwonder:move-complete-event branch from 870ffed to 1b11ab8 Jan 18, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.319% when pulling 1b11ab8 on SteVwonder:move-complete-event into 40a6549 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 18, 2017

This looks good to me, thanks!

@SteVwonder, would you be willing to flesh out the commit message a bit before we merge? E.g. shorten the title to 50 chars, then in the commit message body describe the issue and the resulting solution for posterity? Thanks.

Also I have just a couple very minor line comments I'll add presently to the commit. All very minor and not critical IMO.


lua_stack_call (ctx->lua_stack, "rexecd_exit");
}
if (!exec_rc && flux_reactor_run (flux_get_reactor (ctx->flux), 0) < 0)

This comment has been minimized.

@grondo

grondo Jan 18, 2017

Contributor

For some reason if (!exec_rc .. confused me for a second here, would you be willing to change to if (exec_rc == 0 && for clarity? Might just be me though...

This comment has been minimized.

@lipari

lipari Jan 18, 2017

Contributor

@grondo, if this is to be a convention to follow, it should be added to RFC 7. I use if (something) all the time. And I readily recognize it as a shortcut for if (something != 0). Similarly if (!something) is a valid shortcut for if (something == 0) - at least to my eyes anyway.

This comment has been minimized.

@grondo

grondo Jan 18, 2017

Contributor

Yes, it is technically valid. However, In cases like this where rc < 0 means failure and rc == 0 means success, I find use of if (rc) and if (!rc) a possible source for confusion because my brain tends to promote the variable to a boolean -- moreso when the test is split from the call site and I may have lost a modicum of context.. In this specific case the the line reads as "if not exec_rc" which is much more confusing to me than "if exec_rc is zero".

However, I'm willing to admit this could be my personal hangup and it doesn't matter enough to block merge of this PR.


if (!exec_rc) {

This comment has been minimized.

@grondo

grondo Jan 18, 2017

Contributor

Same as above, I'd prefer if (exec_rc == 0)

@SteVwonder SteVwonder force-pushed the SteVwonder:move-complete-event branch from 1b11ab8 to 4ece754 Jan 18, 2017

wrexecd: delay sending of complete event
Send the 'wreck.state.complete' after job archival as opposed to
before. This is done to prevent a race condition between wrexecd and
other modules.  Before, after wrexecd sent the 'complete' event it
archived the job by modifying the KVS.  Other modules that were
subscribed to the 'complete' event could be activated and read from the
job's KVS directory before/while wrexecd was archiving the job. Now,
modules subscribed to the 'complete' state will only be activated after
wrexecd has finished modifying the KVS.

Closes #728 (see for more info)

@SteVwonder SteVwonder force-pushed the SteVwonder:move-complete-event branch from 4ece754 to 81342b9 Jan 18, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.162% when pulling 81342b9 on SteVwonder:move-complete-event into 3e05f58 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage decreased (-0.006%) to 76.192% when pulling 81342b9 on SteVwonder:move-complete-event into 3e05f58 on flux-framework:master.

@SteVwonder

This comment has been minimized.

Copy link
Member Author

SteVwonder commented Jan 18, 2017

@grondo: I modified those if statements to include the more explicit == 0. I also updated the commit message and rebased.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jan 18, 2017

@SteVwonder: great thanks, merging!

@grondo grondo merged commit e2528d1 into flux-framework:master Jan 18, 2017

4 checks passed

codecov/patch 87.50% of diff hit (target 75.91%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +11.58% compared to 3e05f58
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 76.192%
Details

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

@SteVwonder SteVwonder deleted the SteVwonder:move-complete-event branch Feb 16, 2019

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.