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: improve job.state.priority callback #425

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

cmoussa1
Copy link
Member

@cmoussa1 cmoussa1 commented Feb 26, 2024

Background

Continuing the improvement process on the priority plugin. The callback for job.state.priority uses a manual lookup for associations in the case where we need to re-look up a user's accounting information, but accounting.cpp has a cleanly defined implementation of this now.


This PR reformats the job.state.priority callback to make use of the new external lookup function defined in job.state.priority. It improves some of the comments throughout the function and adds new ones. Finally, it adjusts the test that checks for an exception message in job.state.priority to account for the new message format (it matches the exception message in job.validate).

Fixes #424

Problem: The callback for job.state.priority uses a manual lookup for
associations in the case where we need to re-look up a user's accounting
information, but accounting.cpp has a cleanly defined implementation of
this now.

Reformat the job.state.priority callback to make use of the new external
lookup function defined in job.state.priority. Improve some of the
comments throughout the function and add new ones. Adjust the test that
checks for an exception message in job.state.priority to account for the
new message.
@cmoussa1 cmoussa1 added improvement Upgrades to an already existing feature plugin related to the multi-factor priority plugin labels Feb 26, 2024
@cmoussa1 cmoussa1 changed the title [WIP] plugin: improve job.state.priority callback plugin: improve job.state.priority callback Feb 27, 2024
@cmoussa1 cmoussa1 marked this pull request as ready for review February 27, 2024 16:41
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.

LGTM

@cmoussa1
Copy link
Member Author

Thanks! Setting MWP

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Merging #425 (fc89b18) into master (5743ce4) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   82.83%   82.76%   -0.08%     
==========================================
  Files          23       23              
  Lines        1410     1398      -12     
==========================================
- Hits         1168     1157      -11     
+ Misses        242      241       -1     
Files Coverage Δ
src/plugins/mf_priority.cpp 83.69% <100.00%> (-0.29%) ⬇️

@mergify mergify bot merged commit a4ed602 into flux-framework:master Feb 27, 2024
13 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.

job.state.priority: use new external function for association lookup, general function improvement
2 participants