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: add set- and get-property support to match service #513

Merged
merged 2 commits into from Sep 20, 2019

Conversation

@tpatki
Copy link
Member

tpatki commented Sep 18, 2019

Resolve issue #494, get/set property is now supported in flux resource through RPC.

@tpatki tpatki requested a review from dongahn Sep 18, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 18, 2019

Codecov Report

Merging #513 into master will increase coverage by 0.1%.
The diff coverage is 86.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #513     +/-   ##
=========================================
+ Coverage   75.95%   76.05%   +0.1%     
=========================================
  Files          63       63             
  Lines        6442     6503     +61     
=========================================
+ Hits         4893     4946     +53     
- Misses       1549     1557      +8
Impacted Files Coverage Δ
resource/modules/resource_match.cpp 75.66% <86.88%> (+1.76%) ⬆️

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 17553f7...f6dfbf1. Read the comment docs.

@tpatki tpatki force-pushed the tpatki:issue494 branch from 4342163 to 2569b91 Sep 18, 2019
resource/modules/resource_match.cpp Outdated Show resolved Hide resolved
resource/modules/resource_match.cpp Outdated Show resolved Hide resolved
resource/modules/resource_match.cpp Outdated Show resolved Hide resolved

if (it == ctx->db.by_path.end ()) {
errno = EINVAL;
flux_log (h, LOG_DEBUG, "Couldn't find %s in resource graph.",

This comment has been minimized.

Copy link
@dongahn

dongahn Sep 18, 2019

Contributor

flux_log_error instead?

resource/modules/resource_match.cpp Outdated Show resolved Hide resolved
resource/modules/resource_match.cpp Outdated Show resolved Hide resolved
t/scripts/flux-resource Outdated Show resolved Hide resolved
t/scripts/flux-resource Outdated Show resolved Hide resolved
t/scripts/flux-resource Outdated Show resolved Hide resolved
t/scripts/flux-resource Outdated Show resolved Hide resolved
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

@tpatki: Good direction. The code looks pretty good to me. I just had some minor suggestions inlined above. Before this can go in, we will need some test cases though.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

@tpatki: I just noticed that the title for this PR is WIP: Issue494. I think your PR will appear more descriptive if you change it something like:

WIP: resource: Add set- and get-property support to match service

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Sep 18, 2019

@dongahn: I thought I had resolved the indentation issue with vim :retab but it didn't address all of them. I will fix it again :D

Any suggestions on which other test cases to add? I noticed that the codecov checks failed, not sure how to address that. Here are the current tests -- not sure if you took a look.

One thing I've noticed about the flux-resource script is that it doesn't check for missing arguments anywhere. For example, if you specify flux resource cancel instead of flux resource cancel <jobid>, it doesn't return an error value. I was testing this issue with the one check that you pointed out was redundant (at line #894 and #905). I need to remove that property_key.empty () check.

Surprisingly, flux-resource doesn't fail gracefully for a missing argument when I test locally, no matter what the command is (I tested cancel and info with t4003). Thoughts on how to address this? I think we need to address this in the python script.

@tpatki tpatki changed the title WIP: Issue494 WIP: resource: Add set- and get-property support to match service Sep 18, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

Any suggestions on which other test cases to add? I noticed that the codecov checks failed, not sure how to address that. Here are the current tests -- not sure if you took a look.

Actually, I missed this commit this morning. I will take a look to see if this addresses my concerns.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

BTW, please rebase your PR to the latest master and forced a push.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

One thing I've noticed about the flux-resource script is that it doesn't check for missing arguments anywhere. For example, if you specify flux resource cancel instead of flux resource cancel , it doesn't return an error value. I was testing this issue with the one check that you pointed out was redundant (at line #894 and #905). I need to remove that property_key.empty () check.

I see. Could you quickly create an issue ticket for this. IMO, this should be fixed at the front end and keeping the service code lean and mean.

@tpatki tpatki force-pushed the tpatki:issue494 branch from 2569b91 to fc63a16 Sep 18, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Sep 18, 2019

@dongahn: resolved all of your comments. One thing to note is that when we change from flux_log to flux_log_error we see the error message on the console when running the test case. Previously, this would only show for debug. I am hoping this is ok.

Hopefully I resolved all indentation and 80-char issues, it looks fine at my end in vim now. Let me know if I missed any.

resource/modules/resource_match.cpp Outdated Show resolved Hide resolved
resource/modules/resource_match.cpp Outdated Show resolved Hide resolved
t/t4006-properties.t Show resolved Hide resolved
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

@tpatki: This looks great to me. I only found a few indentation issues. @SteVwonder: Would you mind briefly looking at this PR to see if I missed anything?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 18, 2019

@tpatki: At this point, I would remove WIP from the PR title. Thanks.

Copy link
Member

SteVwonder left a comment

Generally LGTM! Thanks @tpatki for putting this together along with extensive tests.

When I go to the coverage site, it thinks that not a single line of the get or set callback functions is run. I was confused at first, but I think the reason is that the t/t4006-properties.t test is never run. It needs to be added to the TESTS variable in t/Makefile.am (https://github.com/flux-framework/flux-sched/blob/master/t/Makefile.am#L30), and then it will be run automatically by make check.

if (ctx->db.resource_graph[v].properties.find (property_key)
!= ctx->db.resource_graph[v].properties.end ()) {
ctx->db.resource_graph[v].properties.erase (property_key);
}

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder Sep 19, 2019

Member

Today I learned that the map.insert method will return a pair that contains true or false depending on if the insert succeeded or failed due to a pre-existing entry. The pair will also contain an iterator pointing to the newly inserted object or the pre-existing object (depending on insertion success/failure). So if you wanted to avoid the find call, when the insert returns false, you can call erase directly on the returned iterator and re-try the insert.

I do not think this change is strictly necessary though, just wanted to pass along something new that I learned while looking at this.

Docs and example code: http://www.cplusplus.com/reference/map/map/insert/

This comment has been minimized.

Copy link
@tpatki

tpatki Sep 19, 2019

Author Member

This is useful to know. I won't change this for now but I will certainly keep this in mind the next time I use erase/insert (as in, in terms of performance, either you have two insert calls or a find + insert call...do we know if one is better than other?)

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder Sep 19, 2019

Member

In the case of the two inserts, you only have to do the second one when the first fails. In the find + insert case, you always have to do both. So I (naively) assume the former is a little faster.

This comment has been minimized.

Copy link
@tpatki

tpatki Sep 19, 2019

Author Member

Good point. Changing this now, shall update the PR shortly.

flux resource get-property /tiny0/rack0/node0 prop4 >> sp.2 &&
flux resource get-property /tiny0/rack0/node0 prop5 >> sp.2 &&
echo "prop1 = a\nprop2 = foo\nprop3 = 123\n\
prop4 = bar\nprop5 = baz" > expected &&

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder Sep 19, 2019

Member

One more weird indentation on Github's diff window (not sure what it looks like in a text editor)

This comment has been minimized.

Copy link
@tpatki

tpatki Sep 19, 2019

Author Member

For this one, "echo" adds in a tab if I indent it because it is still part of the quotes, which causes the test case to fail as the output/expected don't match. Any ideas on what can be done to avoid that? I considered adding a separate 'expected' file in data/ somewhere, but wasn't sure if that's the best approach...

This comment has been minimized.

Copy link
@grondo

grondo Sep 19, 2019

Contributor

the idiomatic way to accomplish what you want in sharness tests is to use a here-doc instead of echo, with an appended minus sign so that leading tabs are ignored, e.g.

    cat <<-EOF >expected &&
    prop1 = a
    prop2 = foo
    prop3 = 123
    prop4 = bar
    prop5 = baz
    EOF
    ...

There are a number of examples to copy in the flux-core sharness tests in case it is helfpul

This comment has been minimized.

Copy link
@tpatki

tpatki Sep 19, 2019

Author Member

Thanks, @grondo! Changing this in the test case now.

@tpatki tpatki force-pushed the tpatki:issue494 branch from fc63a16 to 7b42f30 Sep 19, 2019
@tpatki tpatki changed the title WIP: resource: Add set- and get-property support to match service resource: Add set- and get-property support to match service Sep 19, 2019
@tpatki tpatki changed the title resource: Add set- and get-property support to match service resource: add set- and get-property support to match service Sep 19, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Sep 19, 2019

@SteVwonder, @dongahn: Thank you for all the excellent comments and feedback on this!

I just realized that the indentation issues occur when I commit from the docker setup I have, so I'll make sure I commit from the terminal that isn't running docker. It's a weird issue. I hope I've gotten all of them taken care of now. :)

Good catch on the t/Makefile.am! I had edited that in my setup so make check worked for me, but had forgotten to add it to the commit. Fixed. Hopefully it'll fix the code coverage issue.

@tpatki tpatki force-pushed the tpatki:issue494 branch 4 times, most recently from 0b74446 to daf3cca Sep 19, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Sep 19, 2019

@grondo, @SteVwonder:
Running into a strange issue when I try using here-doc in the tests. It seems like I have to use tabs for here-doc to work, but this breaks the recommended indentation of tab=4 spaces for sched. If I use ts=4 sw=4 expandtab to fix the indentation and change to 4 spaces, the tests fail as here-doc relies on tabs. Also, despite setting ts = 4 noexpandtab, the settings are lost and revert to tab=8 spaces once I exit vim and try a cat on the test file.

I see that the flux-core tests (t2201) are indeed indented with tab=8. I am doing the same for sched for this one test file (t4006), even though we typically indent with tab=4 spaces.

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Sep 19, 2019

Running into a strange issue when I try using here-doc in the tests. It seems like I have to use tabs for here-doc to work

Is here-doc in a .am file? I have found that in certain scenarios, a tab is required by automake/make to work. So if that is the case, I wouldn't worry about converting it to a space.

@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Sep 19, 2019

Is here-doc in a .am file? I have found that in certain scenarios, a tab is required by automake/make to work. So if that is the case, I wouldn't worry about converting it to a space.

It's in t4006-properties.t.
Needs tabs for it to work, so if we use a 4-space expandtab causes the tests to fail. Like you said, because the tab is required, I wouldn't worry about changing to spaces. I see similar approach in the flux-core tests due to the use of here-doc (eg t2201).

Copy link
Member

SteVwonder left a comment

LGTM! Travis is happy as is the coverage tool. I'm happy to merge this whenever @tpatki. Do you have anything else you want to push? If not, I'll hit the button.

@tpatki tpatki force-pushed the tpatki:issue494 branch from daf3cca to f6dfbf1 Sep 19, 2019
@tpatki

This comment has been minimized.

Copy link
Member Author

tpatki commented Sep 19, 2019

@SteVwonder: just pushed the optimization on insert. Hopefully travis will be green soon and then we can merge! :)

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 20, 2019

Great job @tapasya and thanks @SteVwonder for your thorough review! Merging.

@dongahn dongahn merged commit a317aad into flux-framework:master Sep 20, 2019
3 checks passed
3 checks passed
codecov/patch 86.88% of diff hit (target 75.95%)
Details
codecov/project 76.05% (+0.1%) compared to 17553f7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.