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

flux-event: support raw payloads, pub --loopback, and man page #1488

Merged
merged 9 commits into from
Apr 26, 2018

Conversation

garlick
Copy link
Member

@garlick garlick commented Apr 26, 2018

This was peeled off of pr #1486 and is based on top of it (will rebase once that gets merged).

This enhances the flux-event command. In addition to general cleanup, it adds support for events with raw (non-JSON) payloads, and a flux event pub --loopback option that waits for published events to be received before exiting. It also adds a manual page for the command and a couple of tests.

@coveralls
Copy link

coveralls commented Apr 26, 2018

Coverage Status

Coverage decreased (-0.008%) to 79.017% when pulling 02a679a on garlick:event_cmd_cleanup into 2611b9f on flux-framework:master.

@codecov-io
Copy link

codecov-io commented Apr 26, 2018

Codecov Report

Merging #1488 into master will decrease coverage by <.01%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #1488      +/-   ##
==========================================
- Coverage   78.72%   78.71%   -0.01%     
==========================================
  Files         164      164              
  Lines       30429    30475      +46     
==========================================
+ Hits        23955    23989      +34     
- Misses       6474     6486      +12
Impacted Files Coverage Δ
src/cmd/flux-event.c 69.78% <69.23%> (+2.04%) ⬆️
src/broker/module.c 83.79% <0%> (-1.4%) ⬇️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/broker/content-cache.c 73.43% <0%> (-1.3%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/common/libflux/handle.c 83.66% <0%> (-0.25%) ⬇️
src/common/libflux/reactor.c 93.73% <0%> (+0.28%) ⬆️
src/cmd/flux-module.c 85.36% <0%> (+0.3%) ⬆️
src/common/libflux/msg_handler.c 87.36% <0%> (+0.72%) ⬆️
src/connectors/local/local.c 88.14% <0%> (+0.74%) ⬆️
... and 4 more

@grondo
Copy link
Contributor

grondo commented Apr 26, 2018

Ok, #1486 is merged. Once this one is rebased I'll merge it.

@grondo grondo mentioned this pull request Apr 26, 2018
garlick added 9 commits April 26, 2018 10:00
In the pub subcommand:
- tidy up parsing of topic, payload
- skip redundant JSON parse check (flux_event_encode does it)
- drop json-c dependency
Add pub --raw option, which causes payload argument,
if any, to be interpreted as raw data not JSON.
In the sub subcommand:
- structure event_sub_register() like event_pub_register()
- don't reuse 'n' variable
- simplify subscribe/unsubscribe logic
- eliminate unnecessary 'raw' variable
Problem: "flux event sub --raw" produces the same
result as "FLUX_HANDLE_TRACE=1 flux event sub",
and is confusing given the addition of the
"flux event pub --raw" option.

Drop the sub --raw option.
It's a bit cleaner to use a reactor callback to process
events in "flux event sub" subcommand.

Drop comment about cleaning up on SIGINT, as unsubscribing
is more of a formality anyway.  It is handled automatically
on disconnect from the broker.
If pub --loopback is specified, subscribe to the published
event, publish it, and wait for it to be received before exiting.
Add some coverage for event encode/decode by publishing
events with payloads that are empty, JSON, and raw,
in --loopback mode.
@garlick garlick force-pushed the event_cmd_cleanup branch from 33b27ae to 02a679a Compare April 26, 2018 14:02
@grondo
Copy link
Contributor

grondo commented Apr 26, 2018

Coverage is low but I think we should still merge this.
FYI, it is handling of raw payload in flux-event sub that doesn't seem to have coverage (i.e. make_printable and the else if flux_event_decode_raw() block in flux-event sub)

@garlick
Copy link
Member Author

garlick commented Apr 26, 2018

I don't mind working on that a bit if you want to merge the other one first.

@grondo
Copy link
Contributor

grondo commented Apr 26, 2018

I leave it up to you 😀

@garlick
Copy link
Member Author

garlick commented Apr 26, 2018

I'd say merge this one as is and I'll see if I can address the coverage in the final event PR that I haven't posted yet.

@grondo grondo merged commit 866f4c7 into flux-framework:master Apr 26, 2018
@grondo
Copy link
Contributor

grondo commented Apr 26, 2018

Got it!

@garlick garlick deleted the event_cmd_cleanup branch April 26, 2018 16:20
@grondo grondo mentioned this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants