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

Add --bootstrap option to flux-start #990

Merged
merged 8 commits into from Feb 24, 2017

Conversation

Projects
None yet
6 participants
@morrone
Copy link
Contributor

morrone commented Feb 24, 2017

Add --bootstrap option to flux-start, preceded by a number of cleanup patches.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.223% when pulling 5859996 on morrone:flux-start into 3d0fa0f on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 24, 2017

Codecov Report

Merging #990 into master will decrease coverage by -0.02%.
The diff coverage is 94.56%.

@@            Coverage Diff             @@
##           master     #990      +/-   ##
==========================================
- Coverage   75.96%   75.95%   -0.02%     
==========================================
  Files         152      152              
  Lines       25946    25954       +8     
==========================================
+ Hits        19710    19713       +3     
- Misses       6236     6241       +5
Impacted Files Coverage Δ
src/cmd/flux-start.c 73.88% <94.56%> (+0.41%)
src/modules/connector-local/local.c 71.8% <ø> (-1.87%)
src/common/libflux/handle.c 85.37% <ø> (-0.27%)
src/common/libflux/message.c 83.84% <ø> (-0.27%)
src/modules/kvs/kvs.c 80.53% <ø> (-0.13%)
src/common/libflux/rpc.c 90.97% <ø> (+0.75%)
src/common/libutil/log.c 86.27% <ø> (+9.8%)

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 3d0fa0f...96d9224. Read the comment docs.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 24, 2017

No time to look in detail right now - just quick comments:

I see at least one functions with no paramters prototyped like foo (). I lean towards foo (void). In fact I thought c99 would complain but apparently not since travis is passing. Thoughts? (If it's not a c99 thing then I suppose its a matter for RFC 7 unless we don't care).

The cleanup in here looks fine in general

It looks like flux start is still backwards compatible with the way we've been using it?

morrone added some commits Feb 22, 2017

flux-start: Drop pretense that "struct context" is not a global
"struct contect" is really just a global.  Doing a single malloc in main()
and then passing it to every function doesn't really change that fact, it just
obfuscates it.  So lets just make it a global and then start down the path
of reducing its size.
flux-start: Use macro for default constant
Use a macro for the default constant killer timeout default rather
than a variable (which implies that it might change).
flux-start: Add --bootstrap method.
Add the --bootstrap option to allow specifying a specific instance
bootstrap method.  The initial two methods are "selfpmi" and "pmi",
which are just explicit names for existing methods.  selfpmi is the
new name for the method where flux-start itself starts a PMI server,
and then launches brokers use PMI to bootstrap using said server.
With "pmi", flux-start just assumes a PMI environment is available
and execs a single broker directly.

Documentation is updated and tests are added for this new interface.

Fixes #952
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Feb 24, 2017

OK, I made the update to make the function declarations actual prototypes with (void).

Yes, this just implements #952.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.01%) to 76.226% when pulling 96d9224 on morrone:flux-start into 3d0fa0f on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Feb 24, 2017

In general, is the pattern of bundling program state into a context struct, and passing that around to ancillary functions considered poor form, or was this a special case in this PR?

I tend to like avoiding the "global context" because it makes it more difficult to see which functions are using or require bits of the program state, so I have made this same choice (mistake?) many times. It would help if there was some other guidance as to why its usage here was unpalatable.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 24, 2017

In general, is the pattern of bundling program state into a context struct, and passing that around to ancillary functions considered poor form, or was this a special case in this PR?

I also think having some guidance here would be useful. At least for a C API, I tend to like the style of passing around an API-level context as the opaque object between the client and API implementation.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 24, 2017

We just had a mini discussion on this in our meeting. Let me see if I can summarize:

First we are talking about a single program context here, not an API context. The program context is effectively global if it's passed to every function in the program.

When the program context is passed as a parameter to functions instead of the one or two things in it that they actually need, then the cues about the extent of the function's inputs and outputs normally present in the parameter list are lost.

When state is placed in the context instead of being created/destroyed at the level of abstraction that it's needed, cues about when it can change and how it is used are lost.

Finally, (and this might be less unanimously agreed upon by the group), mallocing a context that is only used once confuses the casual reader of the code about its intended use. Can there be more than one?

Did I cover the main points?

These changes look good - merging.

@garlick garlick merged commit fd3ebe0 into flux-framework:master Feb 24, 2017

4 checks passed

codecov/patch 94.56% of diff hit (target 75.96%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +18.59% compared to 3d0fa0f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 76.226%
Details
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Feb 24, 2017

Good summary!

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Feb 24, 2017

Makes sense!

@morrone morrone deleted the morrone:flux-start branch Feb 25, 2017

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 25, 2017

A downside of making the context actually global is now you don't get any cues from a function's parameter list that it's been weened off of the context...

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

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.