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

plugin: move flux_respond () to end of functions #431

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

cmoussa1
Copy link
Member

Problem

There are a couple callbacks in the plugin that call flux_respond () before they are done processing.


This PR moves these calls to just before the functions call return.

@cmoussa1 cmoussa1 added improvement Upgrades to an already existing feature plugin related to the multi-factor priority plugin labels Mar 15, 2024
Copy link
Member

@jameshcorbett jameshcorbett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine but does have any effect other than changing the order of log messages in the event of an error?

@cmoussa1 cmoussa1 changed the title [WIP] plugin: move flux_respond () to end of functions plugin: move flux_respond () to end of functions Mar 18, 2024
@cmoussa1 cmoussa1 marked this pull request as ready for review March 18, 2024 15:21
@cmoussa1
Copy link
Member Author

I don't think so. I think this call is supposed to be placed at the end of the callback in the event where there is no error. This commit was actually part of some work on an old (and now closed PR) that was pointed out by @grondo - maybe he could confirm that this is still ok?

@cmoussa1 cmoussa1 force-pushed the move.flux-respond branch from 124a50e to 5c24d21 Compare March 18, 2024 15:30
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Merging #431 (5c24d21) into master (b8d960c) will not change coverage.
The diff coverage is 50.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #431   +/-   ##
=======================================
  Coverage   83.39%   83.39%           
=======================================
  Files          23       23           
  Lines        1457     1457           
=======================================
  Hits         1215     1215           
  Misses        242      242           
Files Coverage Δ
src/plugins/mf_priority.cpp 84.21% <50.00%> (ø)

@cmoussa1
Copy link
Member Author

@grondo if you have some time this week or next, could you please give this PR a look?

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice cleanup!

Problem: There are a couple callbacks in the plugin that call
flux_respond () before they are done processing.

Move these calls to just before the functions call "return".
@cmoussa1
Copy link
Member Author

Thanks!! setting MWP here

@cmoussa1 cmoussa1 requested review from jameshcorbett and removed request for jameshcorbett August 28, 2024 20:09
@mergify mergify bot merged commit dca3471 into flux-framework:master Aug 28, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Upgrades to an already existing feature merge-when-passing plugin related to the multi-factor priority plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants