weighted_tree: make load_accounting_db public #103

cmoussa1 · 2021-02-10T19:11:55Z

note: this PR is built on top of #97 and should not be reviewed/merged until that one lands. I'll keep it as a draft PR for now.

PR #97 adds the ability to create a populated weighted tree from a flux-accounting DB for testing purposes. That functionality will be required moving forward to handle #63.

To make this easier, this PR pulls the load_accounting_db.* files out of the "test" directory and up a level into weighted_tree/.

Fixes #102

dongahn · 2021-02-18T20:16:19Z

Just a quick comment: did you want to put the new public function into the Flux:: accounting namespace?

cmoussa1 · 2021-02-18T20:27:40Z

Hm, I'm not sure - I hadn't thought about it. My idea in #104 was to leverage the code written in #97 and use it to create the hierarchical output. Would it then make sense to add the new public function to the namespace?

cmoussa1 · 2021-02-19T20:48:19Z

OK, now that #97 has landed, I think this is ready for review.

dongahn · 2021-02-19T22:35:47Z

@cmoussa1: I understand you want to create a public interface that will instantiate a weighted tree object with the given DB. I have two concerns: 1) by placing this file to the weighted_tree library code, we are breaking a good separation of concern. I think it perhaps makes better sense to keep libweighted_tree to only contain the code that concerns "fairness algorithm code" not the source of the data; 2) the load source files are really isn't shaped well as a public API. If nothing else, It has a bunch of internal functions with global linkage etc.

I'd suggest the following.

Add a reader directory under fairness
Add DB-based reader as a C++ class with a clean set public APIs. This should include adding error message support which can be used to capture the error messages from sqlite as we discussed at our team meeting yesterday. For the general OOP design pattern, I believe you can model after our resource reader structure for Fluxion -- https://github.com/flux-framework/flux-sched/tree/master/resource/readers)
Add the DB-based unit test cases in that "reader" directory

Hope this makes sense.

cmoussa1 · 2021-02-23T18:01:26Z

Thanks for your feedback and suggestions @dongahn - I pushed up a couple of changes that moves the load-DB code and its unit test file and sample DB's into their own directory under fairness/, called reader/. I also went ahead and changed the names of the source files to data_reader_db.*.

This should include adding error message support which can be used to capture the error messages from sqlite as we discussed at our team meeting yesterday.

I think I could use some more clarification here. Are you thinking that this code could add additional context when an error occurs while interacting with the SQLite database? To the best of my knowledge, the sqlite3_* error message will be captured and sent to cerr and the appropriate errno will be set, such as in get_sub_banks():

rc = sqlite3_bind_text (c_shrs, 1, bank_name.c_str (), -1, NULL);
if (rc != SQLITE_OK) {
    std::cerr << sqlite3_errmsg (DB) << std::endl;
    errno = EPROTO;

    return nullptr;
}

But maybe there is something more this code could do that I don't know about. Maybe the "Unable to fetch data" error messages could use more context, since there are multiple locations where a failed data fetch could occur.

dongahn · 2021-02-23T18:11:09Z

@cmoussa1: please see the error message string support here.

If you have something like that in the reader base class, you can change the above code:

rc = sqlite3_bind_text (c_shrs, 1, bank_name.c_str (), -1, NULL);
if (rc != SQLITE_OK) {
    m_err_msg += sqlite3_errmsg (DB) + std::string (" ");
    errno = EPROTO;
    return nullptr;
}

This way, if the user code of the reader object wants to print out an error message, they can do something like this code

Each user of your code would have its own preferred way to log messages and it won't be pretty if your public API prints out things in stderr/stdout.

Also as I see this, I think EINVAL would be a better catch-all error number than EPROTO...

PR flux-framework#97 adds the ability to create a populated weighted tree from a flux-accounting DB for testing purposes. That functionality will be required moving forward to handle flux-framework#63. To make this easier, pull the load_accounting_db.* source files out of the "test" directory and move them to a new directory "reader", located under "fairness." Rename the load_accounting_db.* files to data_reader_db.*. Also move the unit test file and sample databases to the reader directory. Fixes flux-framework#102

Now that the load_accounting_db source and test code is located in another directory "reader", remove its compilation instructions from the Makefile located in src/fairness/weighted_tree/test/.

SteVwonder · 2021-03-04T23:05:21Z

src/fairness/reader/data_reader_base.cpp

@@ -0,0 +1,39 @@
+/*****************************************************************************\
+ *  Copyright (c) 2020 Lawrence Livermore National Security, LLC.  Produced at


2020 -> 2021

src/fairness/reader/data_reader_base.cpp

src/fairness/reader/data_reader_base.hpp

src/fairness/reader/data_reader_db.cpp

SteVwonder · 2021-03-04T23:18:56Z

src/fairness/reader/data_reader_db.cpp

-        std::cerr << "error opening DB: " << sqlite3_errmsg (DB) << std::endl;
+        std::stringstream msg;
+        msg << "error opening DB: " << sqlite3_errmsg (DB);
+        m_err_msg += msg.str ();


Per @dongahn 's verbal comment, the += is useful for recursive functions, but for single-invocation functions, using = is preferred to ensure stale errors are not included.

SteVwonder · 2021-03-04T23:20:45Z

src/fairness/reader/test/data_reader_db_test01.cpp


 static void test_fairshare_order (const std::string &filename,
                                  const std::vector<std::string> &expected)
 {
    bool bo = true;
    std::shared_ptr<weighted_tree_node_t> root;
+    data_reader_db_t data_reader;


We only have one derived class so far, so this is probably fine, but once we have additional derived classes, we will probably need a factory class like in resource's reader directory.

Ah, thanks for pointing this out. What do you think about me posting a follow-up PR after this one that adds a factory class? I can open an issue to track this.

I also don't mind adding another commit that adds the factory class in this PR if we'd like to have it available from the start.

What do you think about me posting a follow-up PR after this one that adds a factory class? I can open an issue to track this.

Sounds good to me.

cmoussa1 · 2021-03-08T15:44:58Z

Thanks for your review @SteVwonder! I pushed up a couple of fixup commits that I hope address your suggestions:

changed the header to a more minimal header (along with the year)
renamed the function definition in data_reader_base.hpp to just load_accounting_info
changed the assignment to m_err_msg to be just an assignment (=) for single invocation functions, but an append (+=) for recursive functions.

SteVwonder

Generally LGTM! Just one comment on the changes. Thanks @cmoussa1!

src/fairness/reader/data_reader_db.cpp

SteVwonder

👍 LGTM! Thanks @cmoussa1!

cmoussa1 · 2021-03-09T15:22:27Z

Thank you! I'll go ahead and set MWP.

dongahn · 2021-03-09T15:29:40Z

@cmoussa1: just a quick thing. For error message where you use +=, you would need trailing whitespace or newline so if multiple messages will be separated properly. Sorry I jumped in late.

cmoussa1 · 2021-03-09T16:09:02Z

Thanks for pointing that out! I will post an update that add a newline before I set MWP.

dongahn · 2021-03-09T16:11:30Z

@cmoussa1: thanks Chris. I think a newline would be a bit better for users. The user of this API can more easily split the error message using the newline as the token and decide what to do (e.g., printing only the last message, finding uniq messages etc...)

Refactor data_reader_db to be a subclass of data_reader_base. Remove the use of std::cerr and instead append or assign error messages to a class attribute named m_err_msg.

codecov · 2021-03-09T17:52:08Z

Codecov Report

Merging #103 (a322c26) into master (e156381) will increase coverage by 19.10%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #103       +/-   ##
===========================================
+ Coverage   70.46%   89.56%   +19.10%     
===========================================
  Files          16       12        -4     
  Lines        1202      556      -646     
===========================================
- Hits          847      498      -349     
+ Misses        355       58      -297

Impacted Files	Coverage Δ
src/fairness/reader/data_reader_base.hpp	`100.00% <100.00%> (ø)`
src/fairness/reader/data_reader_db.hpp	`100.00% <100.00%> (ø)`
src/fairness/reader/test/data_reader_db_test01.cpp	`98.50% <100.00%> (ø)`
src/fairness/account/account.cpp	`54.54% <0.00%> (-6.07%)`	⬇️
src/fairness/weighted_tree/weighted_tree.cpp	`84.90% <0.00%> (-1.89%)`	⬇️
...ings/python/fluxacct/accounting/print_hierarchy.py
...ython/fluxacct/accounting/job_archive_interface.py
src/cmd/flux-account.py
...c/bindings/python/fluxacct/accounting/create_db.py
... and 3 more

cmoussa1 · 2021-03-09T18:21:47Z

OK, now CI is happy. @dongahn I just pushed up a change to add a newline with m_err_msg += instances.

cmoussa1 · 2021-03-09T18:33:10Z

Thank you! 🎉

cmoussa1 added the improvement Upgrades to an already existing feature label Feb 10, 2021

cmoussa1 added this to the End of February Milestone milestone Feb 10, 2021

cmoussa1 changed the title ~~[WIP] weighted_tree: make load_accounting_db public~~ weighted_tree: make load_accounting_db public Feb 10, 2021

cmoussa1 mentioned this pull request Feb 17, 2021

[WIP] print-hierarchy: add job usage values to output of subcommand #104

Closed

cmoussa1 force-pushed the make.load-accounting-db.public branch 3 times, most recently from f3c7de7 to 79440d3 Compare February 19, 2021 20:17

cmoussa1 marked this pull request as ready for review February 19, 2021 20:47

cmoussa1 requested a review from SteVwonder February 19, 2021 20:48

cmoussa1 force-pushed the make.load-accounting-db.public branch 3 times, most recently from b60f940 to 1d9575b Compare February 23, 2021 17:48

cmoussa1 added 3 commits March 4, 2021 12:23

reader: add reader directory to Makefile

a999b2d

remove load_accounting_db from make

364308f

Now that the load_accounting_db source and test code is located in another directory "reader", remove its compilation instructions from the Makefile located in src/fairness/weighted_tree/test/.

cmoussa1 force-pushed the make.load-accounting-db.public branch 2 times, most recently from 4118d4c to 96fe84b Compare March 4, 2021 21:35

SteVwonder reviewed Mar 4, 2021

View reviewed changes

SteVwonder reviewed Mar 8, 2021

View reviewed changes

src/fairness/reader/data_reader_db.cpp Outdated Show resolved Hide resolved

reader: add data reader base class

678fa03

cmoussa1 force-pushed the make.load-accounting-db.public branch from 9802369 to 955070c Compare March 8, 2021 21:58

SteVwonder approved these changes Mar 8, 2021

View reviewed changes

data_reader_db: refactor to be a subclass

7a47a2e

Refactor data_reader_db to be a subclass of data_reader_base. Remove the use of std::cerr and instead append or assign error messages to a class attribute named m_err_msg.

cmoussa1 force-pushed the make.load-accounting-db.public branch from 955070c to 7a47a2e Compare March 9, 2021 16:31

dongahn added the merge-when-passing label Mar 9, 2021

dongahn approved these changes Mar 9, 2021

View reviewed changes

mergify bot merged commit 6dd0a22 into flux-framework:master Mar 9, 2021

cmoussa1 mentioned this pull request Mar 10, 2021

print-hierarchy: add job usage values to output of flux-accounting DB hierarchy #109

Merged

cmoussa1 deleted the make.load-accounting-db.public branch April 6, 2021 15:05

cmoussa1 mentioned this pull request Apr 6, 2021

Weighted tree library #63

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

weighted_tree: make load_accounting_db public #103

weighted_tree: make load_accounting_db public #103

cmoussa1 commented Feb 10, 2021

dongahn commented Feb 18, 2021

cmoussa1 commented Feb 18, 2021

cmoussa1 commented Feb 19, 2021

dongahn commented Feb 19, 2021 •

edited

Loading

cmoussa1 commented Feb 23, 2021

dongahn commented Feb 23, 2021 •

edited

Loading

SteVwonder Mar 4, 2021

SteVwonder Mar 4, 2021

SteVwonder Mar 4, 2021

cmoussa1 Mar 5, 2021 •

edited

Loading

SteVwonder Mar 8, 2021

cmoussa1 commented Mar 8, 2021

SteVwonder left a comment

SteVwonder left a comment

cmoussa1 commented Mar 9, 2021

dongahn commented Mar 9, 2021

cmoussa1 commented Mar 9, 2021 •

edited

Loading

dongahn commented Mar 9, 2021

codecov bot commented Mar 9, 2021

cmoussa1 commented Mar 9, 2021

cmoussa1 commented Mar 9, 2021

		@@ -0,0 +1,39 @@
		/*****************************************************************************\
		* Copyright (c) 2020 Lawrence Livermore National Security, LLC. Produced at

weighted_tree: make load_accounting_db public #103

weighted_tree: make load_accounting_db public #103

Conversation

cmoussa1 commented Feb 10, 2021

dongahn commented Feb 18, 2021

cmoussa1 commented Feb 18, 2021

cmoussa1 commented Feb 19, 2021

dongahn commented Feb 19, 2021 • edited Loading

cmoussa1 commented Feb 23, 2021

dongahn commented Feb 23, 2021 • edited Loading

SteVwonder Mar 4, 2021

Choose a reason for hiding this comment

SteVwonder Mar 4, 2021

Choose a reason for hiding this comment

SteVwonder Mar 4, 2021

Choose a reason for hiding this comment

cmoussa1 Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

SteVwonder Mar 8, 2021

Choose a reason for hiding this comment

cmoussa1 commented Mar 8, 2021

SteVwonder left a comment

Choose a reason for hiding this comment

SteVwonder left a comment

Choose a reason for hiding this comment

cmoussa1 commented Mar 9, 2021

dongahn commented Mar 9, 2021

cmoussa1 commented Mar 9, 2021 • edited Loading

dongahn commented Mar 9, 2021

codecov bot commented Mar 9, 2021

Codecov Report

cmoussa1 commented Mar 9, 2021

cmoussa1 commented Mar 9, 2021

dongahn commented Feb 19, 2021 •

edited

Loading

dongahn commented Feb 23, 2021 •

edited

Loading

cmoussa1 Mar 5, 2021 •

edited

Loading

cmoussa1 commented Mar 9, 2021 •

edited

Loading