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

libflux: clean up interface for broker attributes #1845

Merged
merged 15 commits into from Nov 19, 2018

Conversation

Projects
None yet
3 participants
@garlick
Copy link
Member

garlick commented Nov 19, 2018

As proposed in #1818, this PR simplifies the API support for broker attributes. I think the main benefit, besides reducing the API surface area, is that the broker's attribute flags are no longer exposed in the API, which should reduce coupling with the broker implementation and allow it to change without affecting the API, when we have time.

After having a go at converting to future based versions of flux_attr_get() and flux_attr_set(), I concluded that the the current versions that hide synchronous RPCs were probably reasonable to keep after all, given how they are used, and the inconvenience of converting to the more complicated interface.

I ran into trouble attempting to make flux_attr_fake() private. Long story, but linking the broker to a static version of libflux did not work out. Instead it is just renamed to flux_attr_set_cacheonly() and labeled as a for testing.

Finally, flux_get_rank() and flux_get_size(), which are just wrappers for flux_attr_get() were folded into attr.[ch], and the remaining function in info.[ch], flux_get_nodeset(), was removed, and its one and only user, flux module, made to do without.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 19, 2018

Codecov Report

Merging #1845 into master will increase coverage by 0.02%.
The diff coverage is 91.04%.

@@            Coverage Diff            @@
##           master   #1845      +/-   ##
=========================================
+ Coverage   79.88%   79.9%   +0.02%     
=========================================
  Files         197     196       -1     
  Lines       35314   35267      -47     
=========================================
- Hits        28210   28180      -30     
+ Misses       7104    7087      -17
Impacted Files Coverage Δ
src/common/libflux/request.c 89.15% <ø> (ø) ⬆️
src/common/libflux/flog.c 91.42% <ø> (ø) ⬆️
src/common/libflux/mrpc.c 86.71% <ø> (ø) ⬆️
src/common/libflux/rpc.c 92.25% <ø> (ø) ⬆️
src/common/libflux/msg_handler.c 90.23% <ø> (ø) ⬆️
src/modules/kvs/kvs.c 65.65% <100%> (ø) ⬆️
src/modules/wreck/wrexecd.c 76.1% <100%> (ø) ⬆️
src/broker/module.c 83.56% <100%> (ø) ⬆️
src/bindings/lua/flux-lua.c 82.29% <100%> (+0.13%) ⬆️
src/modules/aggregator/aggregator.c 80.26% <100%> (ø) ⬆️
... and 15 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 19, 2018

Oops, was this supposed to go in before #1844?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 19, 2018

Nope - I'll rebase and squash the spelling error fix I tacked on.

garlick added some commits Nov 16, 2018

broker/attr: add attr.rm RPC
Problem: the code the attr.set RPC is somewhat convoluted
due to handling of value=NULL, which is interpreted as
a request to remove the attribute.

Simplify the code by adding a distinct attr.rm RPC.
cmd/flux-lsattr: make attr.list RPC directly
Problem: flux_attr_first() and flux_attr_next()
are only used by the flux-lsattr command.

Make flux-lsattr independent by implementing
the attr.list RPC directly.
libflux/attr: drop flux_attr_first(), _next()
Drop attribute iterators that no longer have users.
libflux/attr: drop flags param from flux_attr_get()
Problem: flags are only needed in broker, and in API
internal attribute cache implementation.

Drop 'flags' parameter from flux_attr_get().

Also, if parameters are invalid, fail with EINVAL.

Update users.
libflux/attr: rename and simplify flux_attr_fake()
Problem: the flux_attr_fake() function name
is a bit cryptic, and the flags param must always
be set to FLUX_ATTRFLAG_IMMUTABLE so why have it?

Rename to flux_attr_set_cacheonly() and drop the
flags parameter.

Update users.
libflux/attr: move flags enum to broker
Problem: broker attr flags are defined in the public API,
but they are not used or needed.

Move them to the private broker attr implementation.

FLUX_ATTRFLAG_IMMUTABLE is needed internally by libflux/attr.c,
so just define that one internally with the same bit value
as the broker's definition.
libflux/attr: refactor internals
Clean up the implementation of the internal attribute cache.
The cache serves two purposes:
1) to store values forever that broker indicates are immutable
2) to store other values temporarily so that cache retains ownership
(freed the next time that attribute is looked up)

Rather than storing each value with a struct wrapper that
includes the flag indicating whether or not it is immutable
and putting all values in one zhash_t, create two zhashx_t's,
one for each purpose, and store string values directly in them.
The better size management of zhashx_t vs zhash_t should minimize
the impact of having two of them.  This allows internal code to
be simplified a bit.
testsuite: add missing done_testing() call
Problem: t/lua/t0004-getattr.t always leaves a trash
directory.

Add missing done_testing() call so that test cleans up.
libflux/attr: pull in flux_get_rank(), _size()
Problem: flux_get_rank() and flux_get_size() are just
wrappers around flux_attr_get() and don't warrant having
their own header and c module.

Incorporate into attr.h and attr.c.

The remaining bits of info.[ch] will be dealt with in
a subsequent commit.

Update users.
cmd/flux-module: do not use flux_get_nodeset()
Problem:  flux-module is the only user of
flux_get_nodeset(), but it is overkill for what
flux-module needs.

Replace with simpler code to construct target
nodeset.

The environment variables FLUX_NODESET_ALL and
FLUX_NODESET_MASK are ignored now.  I don't think
they were ever used.

The argument "self" to mean the local rank is no
longer understood, but I don't think this was used
either.

Some refactoring was necessary because the nodeset
string from flux_get_nodeset was a (const char *),
and now we have to free the nodeset that is created.
However, this extra work should be offset by
simplified error handling in the command context.

Update t0003-module.t since expected parse error
output changed.  Add a test of --exclude.

@garlick garlick force-pushed the garlick:attr_cleanup2 branch from 4c98225 to a51d4c2 Nov 19, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Nov 19, 2018

Oops, I seem to have managed to screw up the conflict resolution. Hang on, one more try.

garlick added some commits Nov 19, 2018

libflux/info: drop flux_get_nodeset()
Now that there are no users, drop flux_get_nodeset()
from the public API.
testsuite: expand flux attr command coverage
Add some missing coverage for flux-getattr,
flux-setattr, and flux-lsattr commands
spotted by codecov.

@garlick garlick force-pushed the garlick:attr_cleanup2 branch from a51d4c2 to 00397b8 Nov 19, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 19, 2018

Nice cleanup!

@grondo grondo merged commit cf31737 into flux-framework:master Nov 19, 2018

3 checks passed

codecov/patch 91.04% of diff hit (target 79.88%)
Details
codecov/project 79.9% (+0.02%) compared to a0b0177
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.