plugin: move helper functions for plugin.query callback
#417
Conversation
cmoussa1
added
improvement
|
@grondo if you don't mind could you advise on if I have the right approach with including |
src/plugins/accounting.cpp
Outdated
| free (json_str); | ||
| json_decref (u); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
There's no error checking in this function. json_object for instance can return NULL and so can e.g. json_integer. I'm not sure what exactly would happen though...
Could you maybe create held_job_ids and user_queues, checking if either creation fails, add all the elements to them (checking for errors as you go) and then pack everything into u with a json_pack? You could have the pack steal the references to held_job_ids and user_queues too. That would combine all your error checking into three locations. Plus one at the end to check that the json_dumps succeeds?
There was a problem hiding this comment.
Oh, sure, that's a great suggestion, thanks. Yeah, I think what I could do is move the creation of both json_array ()s first (and check for errors upon creation there), and then pack everything all at once into u with json_pack (). I'll fix this 👍
src/plugins/accounting.cpp
Outdated
| for (const auto& bank : association.second) { | ||
| // bank.second refers to an Association object | ||
| Association a = bank.second; | ||
| json_t *b = json_loads (a.to_json ().c_str (), 0, nullptr); |
There was a problem hiding this comment.
To avoid writing it out to a string and reading it back in again, I wonder if you could have an Association method like to_json_t that returns a json_t and then have the to_json method invoke that method before turning it into a string?
src/plugins/accounting.cpp
Outdated
| json_array_append_new (banks, b); | ||
| } | ||
|
|
||
| json_object_set_new (u, "banks", banks); |
There was a problem hiding this comment.
There's no error checking in this function either and I think another pack might help with error checking rather than setting both userid and banks independently.
There was a problem hiding this comment.
Yes, you're absolutely right here - I think what I'll do is convert the to_json () method to return a json_str * instead of std::string, and this way, I can avoid calling json_loads () altogether.
I'll also change convert_map_to_json () to just use json_pack () like you suggested. 👍
|
Thanks for beginning to take a look at this @jameshcorbett. I've just pushed up some changes to both the |
cmoussa1
changed the title
plugin.query callbackplugin.query callback
src/plugins/accounting.cpp
Outdated
| return nullptr; | ||
| } | ||
| for (const auto &queue : queues) { | ||
| json_array_append_new (user_queues, json_string (queue.c_str ())); |
There was a problem hiding this comment.
I think technically the json_string call and the json_array_append_new should be checked. Like maybe:
json_t *temp;
if (!(temp = json_string (queue.c_str ())
|| json_array_append_new ( ... ) < 0 ){
return nullptr;
}
There was a problem hiding this comment.
There's a similar situation a few lines above.
There was a problem hiding this comment.
Oh neat! I didn't know you could combine checks like this in a single line - that's clever. I'll make these changes
src/plugins/accounting.cpp
Outdated
| "userid", | ||
| association.first, | ||
| "banks", banks); | ||
| if (!u) { |
There was a problem hiding this comment.
if (!u || json_array_append_new (accounting_data, u) < 0) {
src/plugins/accounting.cpp
Outdated
| json_decref (banks); | ||
| return nullptr; | ||
| } | ||
| json_array_append_new (banks, b); |
There was a problem hiding this comment.
Similar comment as below.
|
Just pushed up some changes per @jameshcorbett suggestions - could you give those a quick scan to ensure I understood your suggestion correctly? 😅 |
|
Appreciate it thanks!! @grondo could you advise on if I have the right approach with including |
Problem: the accounting information that is stored in the plugin's internal map cannot be retrieved and printed out in a readable format like JSON. Add a method to the Association class called to_json (), which will convert an Association object to JSON format. Add a helper function to accounting.cpp called convert_map_to_json (), which will convert all of the users/banks in the plugin's internal users map to JSON format and return it as a pointer to a json_t. Add jansson as a library in src/Makefile.am.
Problem: The priority plugin uses multiple helper functions defined in the plugin itself that convert the user/bank data in the users map to JSON format to be returned in the plugin.query callback, but there are now external functions that do this much more cleanly. Remove the helper functions defined in the plugin itself and instead use the external functions defined in association.cpp to convert the accounting data to JSON format.
Problem: The "all_users" variable could be better named to something that represents what it's actually storing, which is the flux-accounting data currently present in the plugin. Rename the variable to "accounting_data".
Problem: The new external functions that convert the accounting data in the plugin's internal map has been updated and now returns all info per user/bank in the map, but the tests in t1019 check for JSON output that matches the old output. Update the expected output to match the new updated JSON objects that are constructed for each user/bank in the plugin.
Problem: None of the expected output of the tests in t1019 have queues in them because no associations belong to any queues when their info is sent to the priority plugin. Add some queues to the flux-accounting DB and add a queue to one of the users so that we can test expected output for the "queues" key-value pair in the expected output.
|
OK, I've just pushed up some changes to add a check for |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #417 +/- ##
==========================================
+ Coverage 81.54% 82.35% +0.81%
==========================================
Files 23 23
Lines 1474 1451 -23
==========================================
- Hits 1202 1195 -7
+ Misses 272 256 -16
|
Background
The plugin defines a number of helper functions inside the plugin itself to help convert flux-accounting data to JSON format. It would significantly clean up the code if this was moved out of the plugin, especially considering it concerns flux-accounting data specifically.
This PR looks to do exactly that. This PR moves the helper functions responsible for creating JSON objects of all of the flux-accounting data in the plugin to
accounting.cpp.In the
Associationclass, I've added a.to_json ()method which converts the object to JSON format. I've also added a new function,converted_map_to_json (), which iterates through all users in the plugin's internal map and creates a JSON object containing all of its information.As a result, the callback for
plugin.queryis also changed to use this new external function, and the previously defined helper functions insidemf_priority.cppare removed since they are no longer needed. You'll notice that the expected output for the tests for this callback have changed slightly, as I've chosen to include pretty much every attribute of eachAssociationobject.TODO/might need further feedback or suggestions
-ljanssontosrc/Makefile.amin order for the unit test file insrc/plugins/testto run and find all of the jansson methods. Is this the right approach? Perhaps there is something else I am missing or doing something completely wrong.