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

aggregator: remove synchronous RPCs and extend to generic values #1535

Merged
merged 4 commits into from May 29, 2018

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented May 24, 2018

This is overall a fairly minor improvement to the aggregator module.

First the remaining synchronous RPCs were removed. Then the int64_t "value" (the target of the aggregate) was replaced with a generic json_t value. This allows identical values to be aggregated other than just integers, including floating-point, strings, and even JSON objects. As long as json_equal() returns true between two values, they will be coalesced by integer identifier.

The "summary statistics" (min, max) are generalized into a summary object, however this is (currently) only supported for real and integer values.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 24, 2018

Codecov Report

Merging #1535 into master will increase coverage by <.01%.
The diff coverage is 83%.

@@            Coverage Diff             @@
##           master    #1535      +/-   ##
==========================================
+ Coverage    78.8%   78.81%   +<.01%     
==========================================
  Files         164      164              
  Lines       30333    30378      +45     
==========================================
+ Hits        23905    23941      +36     
- Misses       6428     6437       +9
Impacted Files Coverage Δ
src/modules/aggregator/aggregator.c 79.86% <83%> (-0.38%) ⬇️
src/common/libkvs/kvs_watch.c 89.69% <0%> (-0.43%) ⬇️
src/common/libflux/message.c 81.17% <0%> (-0.12%) ⬇️
src/broker/overlay.c 74.13% <0%> (ø) ⬆️
src/common/libflux/mrpc.c 86.66% <0%> (+1.17%) ⬆️

@grondo grondo force-pushed the grondo:aggregator-generalize branch 2 times, most recently from 431549f to 51c8f3d May 24, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 24, 2018

I made some changes, squashed and forced a push.

The testing now tests floating point, string, array, and object value types. The flux-aggregate test utility was extended to allow this by accepting values on the cmd line of arbitrary types.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 25, 2018

Coverage Status

Coverage increased (+0.02%) to 79.139% when pulling a3563f9 on grondo:aggregator-generalize into 8bbdc3b on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 28, 2018

This seems like a useful enhancement!

Should we open a separate issue on creating a C API and man pages for the aggregator?

This can go in as far as I'm concerned, just needs a rebase.

grondo added some commits May 23, 2018

aggregator: remove synchronous RPCs
Update the aggregator to remove use of synchronous RPCs in
the forward and sink methods.
aggregator: extend aggregator to any json_t value
Extend the aggregator such that any json_t value can be aggregated.

Values will be aggregated for any json_t types where `json_equal`
returns true. The aggregator will thus now work for integer and double
types, as well as strings, arrays, and even objects.

Summary statistics (max,min) are only supported for integer and
real types. The max and min members are kept at the top level of the
json result written to kvs by the "sink" implementation.
t2100-aggregate.t: Expand aggregator testing
Add tests to ensure floating point, string, and complex object types
work as the value target of aggregates.
cmd/flux-aggregate: allow many types of value arguments
Loosen restrictions on the types of values aggregated by the
flux-aggregate test utility.

Allow value argument on flux-aggregate cmdline to be a string or a
numeric value.

Add a `-e, --execute=CODE` argument to allow values that are created
from arbitrary Lua CODE. This allows generation of arrays and complex
objects for aggregator testing.

@grondo grondo force-pushed the grondo:aggregator-generalize branch from 51c8f3d to a3563f9 May 29, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 29, 2018

Restarted a bunch of stalled builders - looks like a travis issue, not one of ours.

@garlick garlick merged commit 531a47a into flux-framework:master May 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 79.139%
Details

@grondo grondo deleted the grondo:aggregator-generalize branch Jul 27, 2018

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.