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.

Sign up for GitHub

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

Jump to bottom

broker: drop -k-ary option, rename tbon.arity attribute to tbon.fanout #3796

Merged
merged 3 commits into from
Jul 26, 2021
Merged

broker: drop -k-ary option, rename tbon.arity attribute to tbon.fanout #3796

merged 3 commits into from
Jul 26, 2021

Conversation

garlick
Copy link
Member

@garlick garlick commented Jul 23, 2021

As noted in #3795, it is confusing that the tbon.arity attribute can only be changed with the broker --k-ary option. Also we seem to be using "fanout", "arity", and "k" for the same thing.

Drop the option, make the attribute settable, and rename the attribute to tbon.fanout (which seems to me the most clear of the three names).

This will require a fluxion update.

@garlick
overlay/attr: defend against invalid arguments
be543af 
Problem: attribute code does not check 'attr' argument
for NULL before using.

Fail with EINVAL if attr argument is NULL.
chu11
chu11 approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a commit message comment. I also noticed "arity" is in the doc spelling file, dunno if we want to remove it there just in case we ever try to use it again in a doc.

etc/flux.service.in
@@ -7,7 +7,7 @@ TimeoutStopSec=90
KillMode=mixed
ExecStart=@X_BINDIR@/flux broker \
--config-path=@X_SYSCONFDIR@/flux/system/conf.d \
--k-ary=256 \
-Stbon.fanout=256 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the commit message header be "rename tbon.arity"? Preferable is "rename tbon.arity to tbon.fanout", but I assume that would make the line too long.

garlick added 2 commits July 23, 2021 09:54
@garlick
broker: rework fanout options
ddf3e35 
Problem: there is an immutable tbon.arity attribute and
a -k broker option.  Since many attributes are settable
on the broker command line with -S, it is confusing to have
an attribute that can only be set with an option.  Also,
tbon.arity is a strange name for the TBON fanout.

Drop -k,--k-ary=N broker option.
Rename tbon.arity to tbon.fanout.
Allow tbon.fanout to be set on the command line.

Update users, including tests, the systemd unit file,
and flux-sharness.sh.

Fixes flux-framework#3795
@garlick
doc: track changes to fanout options
32ec7de 
Problem: docs refer to -k option and tbon.arity attribute,
but those have been renamed.

Update flux-broker(1) and flux-broker-attributes(7).
Drop 'arity' from the dictionary.
@garlick
Copy link
Member Author

garlick commented Jul 23, 2021

Force pushed a change that drops "arity" from the dictonary and tweaks the commit messages slightly per @chu11.

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #3796 (7f67105) into master (c0250a1) will increase coverage by 0.01%.
The diff coverage is 95.16%.

❗ Current head 7f67105 differs from pull request most recent head 32ec7de. Consider uploading reports for the commit 32ec7de to get more accurate results

@@            Coverage Diff             @@
##           master    #3796      +/-   ##
==========================================
+ Coverage   83.31%   83.33%   +0.01%     
==========================================
  Files         342      342              
  Lines       50948    50967      +19     
==========================================
+ Hits        42447    42473      +26     
+ Misses       8501     8494       -7
Impacted Files Coverage Δ
src/broker/attr.c 84.10% <71.42%> (+0.40%) ⬆️
src/bindings/lua/flux-lua.c 87.37% <85.71%> (ø)
src/broker/boot_config.c 80.18% <100.00%> (+0.09%) ⬆️
src/broker/boot_pmi.c 65.73% <100.00%> (+0.24%) ⬆️
src/broker/broker.c 77.08% <100.00%> (+0.23%) ⬆️
src/broker/overlay.c 89.88% <100.00%> (+1.16%) ⬆️
src/connectors/loop/loop.c 85.48% <100.00%> (ø)
src/modules/resource/reslog.c 74.71% <0.00%> (-1.15%) ⬇️
src/common/libflux/message.c 94.33% <0.00%> (-0.11%) ⬇️
src/modules/cron/cron.c 82.25% <0.00%> (+0.47%) ⬆️
... and 1 more

@chu11
Copy link
Member

chu11 commented Jul 23, 2021

LGTM

@garlick
Copy link
Member Author

garlick commented Jul 23, 2021

This will require a fluxion update.

Huh, I may have been confused. Fluxion is working OK with this change.

@garlick
Copy link
Member Author

garlick commented Jul 23, 2021

Coverage test got canceled after 30m. Restarting.

@grondo
Copy link
Contributor

grondo commented Jul 26, 2021

Coverage test got canceled after 30m. Restarting.

I have noticed that sometimes 30m is not quite long enough for coverage tests. We may have to bump this time limit up soon.

grondo
grondo approved these changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mergify mergify bot merged commit 7a75895 into flux-framework:master Jul 26, 2021
@garlick garlick deleted the tbon_arity branch December 17, 2021 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grondo grondo grondo approved these changes

@chu11 chu11 chu11 approved these changes

Assignees
No one assigned
Labels
merge-when-passing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@garlick @chu11 @grondo