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

Refactor the new scheduling infrastructure code + cleanup #323

Merged
merged 4 commits into from Apr 27, 2018

Conversation

Projects
None yet
4 participants
@dongahn
Copy link
Contributor

dongahn commented Apr 27, 2018

This PR addresses #322.

Refactor Scoring API, Resource Data and Color code.

Now use "using" instead of typedef as we are at C++11.

@dongahn dongahn requested a review from SteVwonder Apr 27, 2018

@dongahn dongahn referenced this pull request Apr 27, 2018

Merged

Add R_lite support #321

@dongahn dongahn force-pushed the dongahn:resource_query_cleanup branch 2 times, most recently from 6698bae to ebcd7ba Apr 27, 2018

dongahn added some commits Apr 26, 2018

resource: Refactor Scoring API code
Improve readability by refactoring the scoring api code
into multiple files.

Move edge comparator classes into fold.hpp

Move edge evaluator classes into edge_eval_api.hpp and .cpp.

Move the definition of score_api_t methods into its own
source file, scoring_api.cpp (except for templated methods).
resource: Refactor resource data code into distinct headers/sources
Move schedule data code into sched_data.[h|c]pp.

Move scheduler infrastructure data code into infra_data.[h|c]pp.

Introduce resource_data.cpp to keep the header file clean.
resource: Use "using" keyword instead of "typedef"
"using" is prefered in C++11 as it has a slight
advantage in templated cases.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 27, 2018

Coverage Status

Coverage decreased (-0.9%) to 75.21% when pulling 2f0034b on dongahn:resource_query_cleanup into 908db96 on flux-framework:master.

@dongahn dongahn force-pushed the dongahn:resource_query_cleanup branch from ebcd7ba to 2f0034b Apr 27, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 27, 2018

Codecov Report

Merging #323 into master will decrease coverage by 0.96%.
The diff coverage is 59.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
- Coverage   74.43%   73.47%   -0.97%     
==========================================
  Files          49       56       +7     
  Lines        9638     9688      +50     
==========================================
- Hits         7174     7118      -56     
- Misses       2464     2570     +106
Impacted Files Coverage Δ
resource/resource_gen_spec.hpp 100% <ø> (ø) ⬆️
resource/scoring_api.hpp 78.94% <ø> (-10.78%) ⬇️
resource/resource_graph.hpp 59.25% <ø> (ø) ⬆️
resource/fold.hpp 14.28% <14.28%> (ø)
resource/infra_data.cpp 35.18% <35.18%> (ø)
resource/resource_data.cpp 52.77% <52.77%> (ø)
resource/sched_data.cpp 60% <60%> (ø)
resource/edge_eval_api.cpp 62.02% <62.02%> (ø)
resource/scoring_api.cpp 68.31% <68.31%> (ø)
resource/color.cpp 71.42% <71.42%> (ø)
... and 16 more

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 908db96...2f0034b. Read the comment docs.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Apr 27, 2018

@SteVwonder: thank you for merging #321. Just rebased to the upstream master.

This PR doesn't add any new code; just restructuring some code for future work. There will be a few PRs like this in the future for this layer before adding new functionalities. Hopefully, this one is straightforward to review...

@SteVwonder SteVwonder merged commit aa24fdf into flux-framework:master Apr 27, 2018

2 of 4 checks passed

codecov/patch 59.54% of diff hit (target 74.43%)
Details
codecov/project 73.65% (-0.78%) compared to 908db96
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.9%) to 75.21%
Details
@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Apr 27, 2018

Yep, no problem! LGTM. Thanks for the explanation on the using keyword (I need to step my C++ game up).

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Apr 27, 2018

Great. Yes I had lunch with a guy who knows a lot about c++ and he confirmed this as well. Thanks @SteVwonder. Should be okay to merge this then.

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