plugin: improve callback for job.validate
#415
Conversation
Problem: There is some functionality in the priority plugin (mf_priority.cpp) that is specific to accessing Association objects in a map data structure. These methods can be abstracted out to the newly created association.* files to clean up some of the plugin code. Add a new function to the association.cpp that handles looking up an Association in the users map, or returning nullptr on failure.
Problem: There are no unit tests for get_association (). Begin to add some unit tests for this function.
Problem: The arguments to the get_queue_info () function include an iterator to an Association object, which is used in the function to access the association's list of permissible queues for validation. This can be confusing to read and would be easier if just the list of permissible queues was passed to the function. Change the argument to the get_queue_info () function from an iterator to the entire Association to just the Association's list of permissible queues. Change the calls to this function to account for the argument change.
cmoussa1
added
improvement
cmoussa1
changed the title
job.validatejob.validate
| @@ -10,3 +10,28 @@ | |||
|
|
|||
| #include "accounting.hpp" | |||
|
|
|||
| Association* get_association (int userid, | |||
| const char *bank, | |||
There was a problem hiding this comment.
Why not take a std::string bank and return an Association& rather than a pointer? You'd need to throw an exception instead of returning nullptr though I think.
There was a problem hiding this comment.
The reason for using char * instead of std::string is because the type of bank when it is unpacked with flux_plugin_arg_unpack () is of type char *, and (to my limited C++ knowledge) there isn't an easy way of converting it to std::string.
The reason I chose to return a pointer to an Association object is because the existence of an Association object is not guaranteed (i.e there could be a user or user/bank combo that does not exist in the flux-accounting DB), and the plugin needs to handle this case, which I don't believe is easily handled if I return a reference (I could be wrong though).
There was a problem hiding this comment.
Couldn't you do something like pass std::string{bank} when you invoke the function, and then take a const reference to it in the function invocation?
And yeah in the case where the object doesn't exist, I don't think you could return a null reference, you'd need to throw something like std::out_of_range.
Anyway, it's fine as it is, it's just my understanding that using raw pointers in C++ is not recommended except in special cases. But I'm not confident in my C++ style so I won't press the point, I think it's good to be merged!
There was a problem hiding this comment.
Thanks! I can look into this further in the near future. My proposal here was just to keep things as similar to what they were beforehand as possible so that it required very little changes within the plugin code itself 😆 . Will set MWP here
src/plugins/mf_priority.cpp
Outdated
| args, | ||
| "cannot find user/bank or " | ||
| "user/default bank entry " | ||
| "for: %i", userid); |
There was a problem hiding this comment.
Maybe add for uid: %i? Just to clarify the number that follows is a uid.
There was a problem hiding this comment.
That's a great suggestion, thanks; will fix this.
Problem: There are now helper functions available for the plugin to make use of to help with lookups and access of the internal map for users and banks, but the plugin does not make use of them. Restructure the validate_cb () function to make use of the new get_user_info () helper function. Change the error message for when a user/bank cannot be found and adjust the respective tests that look for this error message. Improve the comments in this function/add new ones to help with some of the clarity in this function.
cmoussa1
force-pushed
the
issue#413
branch
2 times, most recently
from
a53c53b to
41463a9
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #415 +/- ##
==========================================
+ Coverage 81.36% 81.54% +0.18%
==========================================
Files 22 23 +1
Lines 1465 1474 +9
==========================================
+ Hits 1192 1202 +10
+ Misses 273 272 -1
|
Background
There are now helper functions available for the plugin to make use of to help with lookups and access of the internal map for users and banks, but the plugin does not make use of them.
This PR adds a new standalone helper function to
association.cppcalledget_association (), which is just a lookup function of accounting information for a user ID and an optional bank. On success, the address of theAssociationobject in the plugin's internal map is returned. On failure (if the user ID cannot be found or a particular user ID/bank combination cannot be found),nullptris returned.A couple basic unit tests are also added for
get_association ().I've also added a small commit that changes one of the parameters for a helper function that validates a queue for a given
Associationto expect the list of permissible queues for theAssociationinstead of the entire object. Eventually, I think this might be better to move out of the plugin, but for the sake of keeping this PR's scope as narrow as possible, I'll save that for a later date. :-)Finally, some basic cleanup of
job.validateis added using this new standalone helper function, as well as some improvements to comment structure within the callback.