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

Broker cleanup 2 #1234

Merged
merged 4 commits into from Oct 13, 2017

Conversation

Projects
None yet
4 participants
@morrone
Copy link
Contributor

morrone commented Oct 12, 2017

Additional broker clean up along the path to supporting multiple bootstrap methods.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 12, 2017

@morrone could you please open the issue I requested on monday describing the feature you're working on and (briefly) outline how you plan to get there?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 12, 2017

Coverage Status

Changes Unknown when pulling f01cf74 on morrone:broker_cleanup_2 into ** on flux-framework:master**.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 12, 2017

Codecov Report

Merging #1234 into master will decrease coverage by <.01%.
The diff coverage is 96.49%.

@@            Coverage Diff             @@
##           master    #1234      +/-   ##
==========================================
- Coverage   78.14%   78.14%   -0.01%     
==========================================
  Files         154      154              
  Lines       28665    28680      +15     
==========================================
+ Hits        22401    22411      +10     
- Misses       6264     6269       +5
Impacted Files Coverage Δ
src/broker/attr.c 85.91% <100%> (+0.4%) ⬆️
src/broker/overlay.c 72.85% <100%> (+0.92%) ⬆️
src/broker/broker.c 72.93% <95.34%> (-0.08%) ⬇️
src/common/libflux/keepalive.c 86.66% <0%> (-6.67%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/common/libutil/dirwalk.c 93.57% <0%> (-0.72%) ⬇️
src/broker/module.c 85.19% <0%> (-0.28%) ⬇️
src/bindings/lua/flux-lua.c 81.01% <0%> (-0.18%) ⬇️
src/common/libflux/message.c 81.25% <0%> (-0.12%) ⬇️
src/common/libflux/future.c 88.78% <0%> (ø) ⬆️
... and 4 more
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 12, 2017

@garlick, sure, as soon as I have a plan. :) I'm just unwinding intertwined bits so I can see the forest for the trees. I'll open a place holder issue if that makes you feel better.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 12, 2017

Issue #1236 is now open.

morrone added some commits Oct 12, 2017

broker/overlay: Move temporary variable out of overlay_t
rankstr is really just a temporary variable with no further use.  Move
it locally to the place where it is temporarily used.
broker/overlay: Add set_size and get_[rank|size] functions
If we can retrieve the rank and size from the overlay_t, there will be
less need to maintain unnecessary copies elsewhere.
broker/attr: Add attr_add_[int|uint32] functions
There isn't much need to maintain references to external int and
uint32_t values when those values are marked immutable.  On the contrary,
unless those eternal values are very carefully document we introduce
an opportunity for accidental errors.

Furthermore, it is simpler and more efficient to perform the conversion
into a C string just once rather than implementing a callback that
performs the conversion on every access.

We add these new wrappers to enable cleanup elsewhere in the code.
broker: Move rank and size out of broker_ctx_t
Relocate rank and size from broker_ctx_t into overlay_t.
This keeps the information about the size of the overlay network
and our rank in the overlay network closer to the thing that
would know about and manage the overlay network.

Eventually more will be abstracted away into the overlay portion
of the code, which will make it easier to offer alternate methods
of gathering information to feed to the overlay initialization
code.
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 13, 2017

Rebased.

You know, I really think we should revisit this requirement that all landings have to be rebased. Especially since travis is so incredibly unreliable these days, it would make sense to allow obviously non-conflicting PRs to merge without a rebase.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 13, 2017

OK this all looks fine and I'll merge once travis turns green again.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 13, 2017

Coverage Status

Coverage decreased (-0.01%) to 78.709% when pulling 0e47ed5 on morrone:broker_cleanup_2 into 1893b92 on flux-framework:master.

@garlick garlick merged commit 2bddc2e into flux-framework:master Oct 13, 2017

4 checks passed

codecov/patch 96.49% of diff hit (target 78.14%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +18.34% compared to 1893b92
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 78.709%
Details

@morrone morrone deleted the morrone:broker_cleanup_2 branch Oct 13, 2017

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.