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

Issue1040: Update broker attribute names #1042

Merged
merged 4 commits into from Apr 14, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Apr 13, 2017

Update broker attribute names given recent additions of tbon.endpoint and mcast.endpoint

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 13, 2017

Coverage Status

Coverage increased (+0.02%) to 78.192% when pulling 50fff81 on chu11:issue1040 into e99071f on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 13, 2017

Codecov Report

Merging #1042 into master will decrease coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1042      +/-   ##
==========================================
- Coverage   77.91%   77.91%   -0.01%     
==========================================
  Files         150      150              
  Lines       25686    25684       -2     
==========================================
- Hits        20014    20012       -2     
  Misses       5672     5672
Impacted Files Coverage Δ
src/broker/broker.c 71.91% <60%> (-0.44%) ⬇️
src/cmd/flux-event.c 65.55% <0%> (-1.12%) ⬇️
src/common/libflux/dispatch.c 83.24% <0%> (-0.28%) ⬇️
src/common/libflux/message.c 81.32% <0%> (+0.12%) ⬆️
src/modules/connector-local/local.c 70.49% <0%> (+0.2%) ⬆️
src/common/libutil/veb.c 98.87% <0%> (+0.56%) ⬆️
src/broker/overlay.c 68.26% <0%> (+0.96%) ⬆️

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 bef29dc...fc8fe1e. Read the comment docs.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 13, 2017

Maybe "endpint-uri" should just be "endpoint" (since endpoints in zmq parlance are URI's?)

Were you thinking about removing the old ones in favor of expanding the new ones (tbon-request-uri vs tbon.endpoint, event-uri vs mcast.endpoint?)

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 13, 2017

I kept the -uri mostly out of legacy. If we do axe the -uri, we would probably want to change snoop-uri, local-uri, and parent-uri too.

Personally, I like the idea of being able to see what the user original input via format specifiers vs. what was eventually expanded out. Thus having tbon.endpoint-uri vs just expanding tbon.endpoint out and replacing it with the expanded out value. But perhaps that's just me. I will cave to majority opinion :-)

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 14, 2017

snoop-uri is gone already right (in e1e512f)?

Maybe we need more peers to chime in here :-) At runtime I kind of think the actual values are more interesting than the symbolic values from the command line.

I think i'd be for dropping -uri suffix if the word "endpoint" is there, except for local-uri which is a connector uri not a zeromq endpoint.

My feelings aren't too strong on these things, so if you feel like it should be the other way, I'm willing to go with it.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 14, 2017

Ahhh, snoop-uri is still lingering in the documentation. I'll just add a patch to remove it.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 14, 2017

Realized this when updating the manpage to remove snoop-uri. In my mind, I see attributes that users set (e.g. persist-directory) and attributes they don't set (e.g. session-id). So I guess in my mind I viewed the need to split it up tbon.endpoint and tbon.endpoint-uri to maintain that split. But perhaps my thinking is wrong. Maybe there are other "in & out" attributes.

@chu11 chu11 force-pushed the chu11:issue1040 branch from 50fff81 to 9e79ccc Apr 14, 2017

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 14, 2017

Some attrs track dynamically changing values too, like some of the content ones.

Just thinking pragmatically, would it be better to have one attribute that represents the broker command line rather than separate attributes for the user setting versus the actual for every attribute?

FWIW I generally think about these attributes in the same way I think of kernel sysctl. Possibly it would be worth examining that metaphor in more detail and see if there are some lessons/rules we want to carry into flux.

Also, just for reference, we have #555 which calls for a rework of the internal (broker) attribute interfaces. I kind of like the interface from libflux but the internal interface seems excessively clunky.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 14, 2017

Coverage Status

Coverage remained the same at 78.172% when pulling 9e79ccc on chu11:issue1040 into bef29dc on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 14, 2017

Just thinking pragmatically, would it be better to have one attribute that represents the broker command line rather than separate attributes for the user setting versus the actual for every attribute?

I was just going to suggest the same thing. I think having the broker cmdline available in the session would be useful in other ways as well. A good idea IMO

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 14, 2017

Additionally, if the attribute was reset with flux setattr then the "user setting" value would be out of sync with the actual value, unless you set both (?) which seems to be unnecessary and confusing. However, a cmdline attribute would still be valid, obvious, and perhaps useful.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 14, 2017

I like the idea of the broker command line as a attribute. I'll adjust this PR but make another issue for that feature. It's certainly not something we need now, but would be nice perhaps in the future.

chu11 added some commits Apr 13, 2017

doc/flux-broker-attributes(7): Add missing attrs
Add documentation for event-uri and event-relay-uri.
broker: Alter attribute exports
Alter tbon.endpoint and mcast.endpoint to evaluate out their
user inputs (e.g. calculate out format specifiers) and export
them back to users.

This change removes the necessity for the tbon-request-uri and
event-uri attributes, so they have been removed.

Update documentation and tests appropriately.
broker: Update attribute names
Given recent additions of the tbon.endpoint and mcast.endpoint
attributes, rename other broker attributes for consistency.

tbon-parent-uri -> tbon.parent-endpoint

event-relay-uri -> mcast.relay-endpoint

Update documentation and add tests appropriately.

Fixes #1040

@chu11 chu11 force-pushed the chu11:issue1040 branch from 9e79ccc to fc8fe1e Apr 14, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 14, 2017

Coverage Status

Coverage decreased (-0.002%) to 78.17% when pulling fc8fe1e on chu11:issue1040 into bef29dc on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 14, 2017

fixed up with discussed changes. codecov diff missing coverage is just missed error messages.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 14, 2017

Thanks!

@garlick garlick merged commit 91319ff into flux-framework:master Apr 14, 2017

2 of 4 checks passed

codecov/patch 60% of diff hit (target 77.91%)
Details
codecov/project 77.91% (-0.01%) compared to bef29dc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.002%) to 78.17%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.