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

resource-query: add support for properties #490

Merged
merged 2 commits into from Jul 17, 2019
Merged

Conversation

@tpatki
Copy link
Member

tpatki commented Jul 11, 2019

WIP PR that adds support for set-property and get-property in resource-query.
TODO: Add similar support to flux-resource script and resource-match.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 11, 2019

Codecov Report

Merging #490 into master will increase coverage by 0.09%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   75.46%   75.56%   +0.09%     
==========================================
  Files          60       60              
  Lines        6119     6156      +37     
==========================================
+ Hits         4618     4652      +34     
- Misses       1501     1504       +3
Impacted Files Coverage Δ
resource/utilities/command.hpp 100% <ø> (ø) ⬆️
resource/utilities/command.cpp 67.5% <91.89%> (+5.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12e58f9...7129447. Read the comment docs.

@tpatki tpatki force-pushed the tpatki:issue469 branch from cc784bc to 68b571e Jul 11, 2019
@tpatki tpatki requested a review from dongahn Jul 11, 2019
@tpatki tpatki force-pushed the tpatki:issue469 branch from 68b571e to 68f6416 Jul 11, 2019
{ "set-property", "p", cmd_set_property, "Add a property to a resource: "
"resource-query> set-property resource PROPERTY=VALUE" },
{ "get-property", "g", cmd_get_property, "Get all properties of a resource: "
"resource-query> get-property resource" }, /* May need to be modified to get-property resource PROPERTY*/

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Yeah, I think extending this to query individual property would come in handy later down the road.

string resource_path = args[1];
string property_key = args[2].substr(0, args[2].find("="));
string property_value = args[2].substr(args[2].find("=")+1);
ostream &out = (ctx->params.r_fname != "")? ctx->params.r_out : cout;

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

We will need an error checking for arg[2]. If it is not well-formed with key=value, we should return with EINVAL.

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Also please check bad inputs like

  • "="
  • "=val"
  • "key="

What happens if only "key" is given BTW? Would args[2].find("=") return npos and npos+1 is not good? Would an exception gets raised or undefined behavior?

string property_value = args[2].substr(args[2].find("=")+1);
ostream &out = (ctx->params.r_fname != "")? ctx->params.r_out : cout;

std::map<std::string, std::vector <vtx_t>>::const_iterator it = ctx->db.by_path.find(resource_path);

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Please break the line: more more than 80 characters per line.


std::map<std::string, std::vector <vtx_t>>::const_iterator it = ctx->db.by_path.find(resource_path);
if (it == ctx->db.by_path.end())
out << "Couldn't find path in resource graph." << endl;

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

This should be an error with ENOENT?

out << "Couldn't find path in resource graph." << endl;
else {
vtx_t v = it->second[0];
ctx->db.resource_graph[v].properties.insert(pair<string,string>(property_key,property_value));

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

A knit. Please break the line.
Perhaps more importantly, if property_key is already present, this becomes NOOP. I think you need a check like the following before this statement.

if (ctx->db.resource_graph[v].properties.find (property_key)
     != ctx->db.resource_graph[v].properties.end ()) {
    db.resource_graph[v].properties.erase (property_key);
}
string resource_path = args[1];
ostream &out = (ctx->params.r_fname != "")? ctx->params.r_out : cout;

std::map<std::string, std::vector <vtx_t>>::const_iterator it = ctx->db.by_path.find(resource_path);

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

The same line break comment.

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Oh, also have a space between the method name and opening parenthesis per our coding style guide.


std::map<std::string, std::vector <vtx_t>>::const_iterator it = ctx->db.by_path.find(resource_path);
if (it == ctx->db.by_path.end())
out << "Could not find path in resource graph." << endl;

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

This should be an error case with ENOENT perhaps.

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Maybe you can also add the input path to the msg log?

}

done:
return 0;

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Same comment with rc.

out << "No properties were found for " << resource_path << ". " << endl;
else {
std::map<std::string, std::string>::const_iterator p_it;
for (p_it=ctx->db.resource_graph[v].properties.begin(); p_it!=ctx->db.resource_graph[v].properties.end(); p_it++)

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

Line break; function calling style; and also I will appreciate if you have spaces around the assignment statements.

@@ -0,0 +1,51 @@
#!/bin/sh

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 11, 2019

Contributor

I would suggest to have some mal-formed inputs to ensure your code behaves as expected.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 11, 2019

@tpatki: looks like a great start. Please see my in-line comments. I think the resource infrastructure overall don't have good support for resource properties yet. (e.g., emitters probably don't write properties -- even if they do, I don't think we have test coverage.) So could you also please open a ticket with "tighten up the property support within resource"?

Thank you for being willing to share your WIP!

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 11, 2019

Forget to mention. You probably want to break the commit into at least two commits: one for the code addition and the other for test addition.

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 11, 2019

Thank you so much for the constructive comments, Dong! Yes, I was wondering about error checks too, so thanks for the feedback there, I'll add those in. And once that's done, I'll add in the script/CB.
(It has been slower than expected but an excellent lesson for me in terms of time estimation).

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 11, 2019

And once that's done, I'll add in the script/CB.

I'd suggest you would make an effort to make this a real PR first. And then, the resource-match part as another PR.

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 11, 2019

I'd suggest you would make an effort to make this a real PR first. And then, the resource-match part as another PR.

Sounds good, will do.

@tpatki tpatki force-pushed the tpatki:issue469 branch from 68f6416 to 738a2ca Jul 12, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 12, 2019

@dongahn: Please take a look.
I'll open a separate issue for the script/resource-match and get started on that soon.

ostream &out = (ctx->params.r_fname != "") ? ctx->params.r_out : cout;
int eq_count = count (args[2].begin (), args[2].end (), '=');

if (eq_count == 1) { /* Exactly 1 '=' sign, try to extract LHS and RHS */

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

Perhaps we want to support a value that may contain =? I can imagine a value string containing = like

perf-class=mem=1:cpu=2

Then, mem=1:cpu=2 will be further parsed by your match callback to decide which subclass you want to use?

This comment has been minimized.

Copy link
@tpatki

tpatki Jul 12, 2019

Author Member

How about we do this after the first version of variation-scheduler is in?
As in, we get that first ECP milestone out and then add:
(1) support for adding multiple properties at once (egmem/cpu examples or comma-separated as opposed to writing out "set-property" every time),
(2) support for get-property PROPERTY

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

Adding multi-subclass capability support within variation scheduler is later okay by me. But maybe this code can be slightly revised to support values that contain = to position the future work? (Since you will have more commits anyway?)

This comment has been minimized.

Copy link
@tpatki

tpatki Jul 12, 2019

Author Member

Sure.
The question will be, when is having more than one= ok, and when it isn't. As in, where do we check and reject the malformed inputs: in the matcher or in the set-property?
perf-class=mem=1:cpu=2 versus
perf-class=mem=5==cpu=2:?garbage

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

I would take everything that comes after the first occurrence of = valid at this point of execution as far as it is not empty. That is, how to interpret the value object is really up to the user of that value string. If a garbage is there, the user will raise an exception.

This comment has been minimized.

Copy link
@tpatki

tpatki Jul 13, 2019

Author Member

Got it, and makes sense. The matcher can reject input that it cannot parse, I suppose.
That's an easy fix (= 1 to >= 1) should do it.

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 13, 2019

Contributor

That's an easy fix (= 1 to >= 1) should do it.

Yes. However, a more efficient trick would be just to find the first occurrence instead of count all of them?

   string key;
   string val;
   size_t pos = 0;
   std::string delim = "=";

    if ((pos = args[2].find (delim)) == std::string::npos
         || pos == 0 || pos == args[2].length () - 1) {
            // input error 
    } else {
        key = args[2].substr (0, pos);
        val = args[2].substr (pos+1);
    }

The code not tested...

ostream &out = (ctx->params.r_fname != "") ? ctx->params.r_out : cout;
int eq_count = count (args[2].begin (), args[2].end (), '=');

if (eq_count == 1) { /* Exactly 1 '=' sign, try to extract LHS and RHS */

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

A knit: over 80 characters :-)

}

if ((eq_count != 1) || property_key.empty () || property_value.empty () ) {
out << "Incorrect input format. Please use "

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

I slightly prefer breaking the output into two lines.

ctx->db.by_path.find (resource_path);

if (it == ctx->db.by_path.end ()) {
out << "Couldn't find path" << resource_path <<

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

Seems like you will need whitespaces in the surrounding strings.

}
else {
vtx_t v = it->second[0];
std::pair<std::map<string,string>::iterator,bool> ret;

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

Seems like a deadwood?

ctx->db.by_path.find (resource_path);

if (it == ctx->db.by_path.end ()) {
out << "Could not find path" << resource_path <<

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

Also need spaces in the surrounding strings.

else {
std::map<std::string, std::string>::const_iterator p_it;
for (p_it = ctx->db.resource_graph[v].properties.begin ();
p_it != ctx->db.resource_graph[v].properties.end (); p_it++)

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 12, 2019

Contributor

This appears to exceed 80 characters in my web browser. Yours may render this differently...

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 12, 2019

Tapasya, please close. I made a few minor comments. Please take a look. For the next round, you may want to have your changes as separate commits with a caveat that they will be squashed before this PR lands. That way, reviewers don't have to review the whole things when the PR only requires minor revision. Not a huge issue with this PR since it is pretty small. But this can help quiet bit for large PRs. Thanks!

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 12, 2019

@dongahn: Thanks, Dong, let me make the changes you suggested.
Looks like 80 chars on my eclipse editor is messed up, let me cap off at 70- or so, that way we have more readable code. Is there a nice automated way to check for this?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 12, 2019

I vaguely remember @SteVwonder had clang based style checker... I maybe wrong though.

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 13, 2019

@dongahn: Made the changes you requested.
Let me know what the process to squash is for this to get merged in when it's ready. Thanks!

@@ -223,29 +223,27 @@ int cmd_set_property (resource_context_t *ctx, std::vector<std::string> &args)
string resource_path = args[1];
string property_key, property_value;
ostream &out = (ctx->params.r_fname != "") ? ctx->params.r_out : cout;
int eq_count = count (args[2].begin (), args[2].end (), '=');
size_t pos = args[2].find ('=');

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 13, 2019

Contributor

@tpatki: Thank you. The code looks great. I think there are problems with our editor though. I just realize the indentation is much larger than the rest of the code. Could you please align the indentation with the rest of the code, so the code will look as if one person wrote?

if (eq_count == 1) { /* Exactly 1 '=' sign, try to extract LHS and RHS */
property_key = args[2].substr (0, args[2].find ("="));
property_value = args[2].substr (args[2].find ("=") + 1);
if (pos == 0 || (pos == args[2].size() - 1) || pos == string::npos) {

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 13, 2019

Contributor

A nit. Space after the size () method.

@@ -289,7 +287,7 @@ int cmd_get_property (resource_context_t *ctx, std::vector<std::string> &args)
else {
std::map<std::string, std::string>::const_iterator p_it;
for (p_it = ctx->db.resource_graph[v].properties.begin ();
p_it != ctx->db.resource_graph[v].properties.end (); p_it++)
p_it != ctx->db.resource_graph[v].properties.end (); p_it++)
out << p_it->first << "=" << p_it->second << endl;

This comment has been minimized.

Copy link
@dongahn

dongahn Jul 13, 2019

Contributor

The indentation for this statement is way off. Eclipse?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 13, 2019

@tpatki: Just a few minor final reviews in-lined. Once those are addressed and the commits are squashed, this PR LGTM.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 15, 2019

@tpatki: once the above issues are handled, this PR will need to be rebased with the upstream and pushed.

$ git remote add upstream git@github.com:flux-framework/flux-sched.git
$ git fetch upstream
$ git rebase upstream/master

If all is good (no conflict is expected), then force a push.

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 15, 2019

@dongahn: Yes, will clean this up shortly this afternoon. Sorry, was out of office this morning, just getting back.

@tpatki tpatki force-pushed the tpatki:issue469 branch from 458cefc to 2c0febe Jul 17, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 17, 2019

@dongahn: I fixed the indentation, and I hope that I did the rebase/squash correctly. Let me know if this was ok.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 17, 2019

Thanks.

  • Seems you lost your last commit splits. Now your single commit has both code change and test.

  • Please rebase your PR with upstream master. You can find this by looking at the warning above "This beach is out of sync with the base branch"

Otherwise LGTM.

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 17, 2019

That's weird, I did rebase (that's how I squashed commits). Trying again.

@tpatki tpatki force-pushed the tpatki:issue469 branch from 2c0febe to f217837 Jul 17, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 17, 2019

That's weird, I did rebase (that's how I squashed commits). Trying again.

Maybe you didn't rebase with upstream but origin?

$ git remote add upstream git@github.com:flux-framework/flux-sched.git
$ git fetch upstream
$ git rebase upstream/master

Should rebase your PR against the upstream master.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 17, 2019

I also saw the name of the PR has [WIP] on it. I guess it's the time to remove that tag.

@tpatki tpatki changed the title resource-query: add support for properties (WIP) resource-query: add support for properties Jul 17, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 17, 2019

@dongahn: let me know if all looks good now.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 17, 2019

Seems you lost your last commit splits. Now your single commit has both code change and test.

Hmmm this needs to be addressed?

@tpatki tpatki force-pushed the tpatki:issue469 branch from f217837 to 96efc82 Jul 17, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Jul 17, 2019

Sorry I had missed that -- done.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 17, 2019

@tpatki: Makefile.am change seems to belong to the test commit.

@tpatki tpatki force-pushed the tpatki:issue469 branch from 96efc82 to 7129447 Jul 17, 2019
@tpatki tpatki requested a review from dongahn Jul 17, 2019
@dongahn dongahn merged commit b51c89d into flux-framework:master Jul 17, 2019
3 checks passed
3 checks passed
codecov/patch 91.89% of diff hit (target 75.46%)
Details
codecov/project 75.56% (+0.09%) compared to 12e58f9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Jul 17, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.