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: add flux_event_encode_raw(), flux_event_decode_raw() #1486

Merged
merged 4 commits into from Apr 26, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Apr 24, 2018

This PR fills in some gaps noted in #1474, adding:

Functions for encoding/decoding events with raw payloads:

/* Encode an event message with optional raw payload.
 */
flux_msg_t *flux_event_encode_raw (const char *topic,
                                   const void *data, int len);

/* Decode an event message, with optional raw payload.
 * If topic is non-NULL, assign the event topic string.
 * Data and len must be non-NULL and will be assigned the payload and length.
 * If there is no payload, they will be assigned NULL and zero.
 * Returns 0 on success, or -1 on failure with errno set.
 */
int flux_event_decode_raw (const flux_msg_t *msg, const char **topic,
                           const void **data, int *len);

Functions for encoding and sending events in one go:

/* Send an event message with no confirmation of acceptance by broker.
 * Flags may include FLUX_MSGFLAG_PRIVATE to restrict access to instance owner.
 * If json_str is non-NULL, it is copied to the message payload.
 * Returns 0 on success, or -1 on failure with errno set.
 */
int flux_publish (flux_t *h, int flags, const char *topic,
                  const char *json_str);

/* Same as flux_publish() except payload is encoded using jansson pack
 * style variable arguments.
 */
int flux_publish_pack (flux_t *h, int flags, const char *topic,
                       const char *fmt, ...);

/* Same as flux_publish() except payload is raw.
 */
int flux_publish_raw (flux_t *h, int flags, const char *topic,
                      const void *data, int len);

Man pages and unit tests are updated accordingly.

Some users are converted.

It does not address the need for API function(s) for the cmb.pub RPC, so it does not finish #1474. I thought that was appropriate for another PR.

I still need to add some tests for publishing raw events.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 24, 2018

Coverage Status

Coverage decreased (-0.02%) to 78.983% when pulling 9f6278f on garlick:event_api into 01e2403 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 24, 2018

Codecov Report

Merging #1486 into master will increase coverage by 0.06%.
The diff coverage is 77.9%.

@@            Coverage Diff             @@
##           master    #1486      +/-   ##
==========================================
+ Coverage   78.69%   78.76%   +0.06%     
==========================================
  Files         164      164              
  Lines       30405    30468      +63     
==========================================
+ Hits        23928    23998      +70     
+ Misses       6477     6470       -7
Impacted Files Coverage Δ
src/broker/broker.c 76.92% <100%> (-0.06%) ⬇️
src/bindings/lua/flux-lua.c 81.44% <100%> (-0.14%) ⬇️
src/cmd/flux-module.c 85.18% <100%> (-0.19%) ⬇️
src/common/libjsc/jstatctl.c 72.34% <50%> (-0.02%) ⬇️
src/cmd/flux-kvs.c 81.38% <50%> (-0.06%) ⬇️
src/modules/kvs/kvs.c 66.08% <50%> (+0.04%) ⬆️
src/modules/aggregator/aggregator.c 79.29% <50%> (+0.06%) ⬆️
src/cmd/flux-event.c 70.14% <70%> (+2.4%) ⬆️
src/common/libflux/event.c 92% <93.65%> (+1.67%) ⬆️
src/modules/connector-local/local.c 72.95% <0%> (-1.64%) ⬇️
... and 16 more
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

Ugh, just realized flux_publish() is mismatched with flux_event_subscribe().

I guess I should change this to flux_event_publish(), flux_event_publish_raw(), and flux_event_publish_pack() to keep it balanced.

garlick added some commits Apr 24, 2018

libflux/event: add raw encode/decode functions
Problem: although event messages support raw (non-JSON)
payloads, no API functions exist for encoding/decoding
them.  Such functions do exist for requests and responses.

Add flux_event_encode_raw() and flux_event_decode_raw().
doc/flux_event_decode(3): add raw encode/decode
Add man page entries for flux_event_encode_raw() and
flux_event_decode_raw().
build: make clean misses some man3 stubs
Problem: make clean leaves some man(3) stub files
in the working directory.

A commented MAN3_FILES_SECONDARY entry with a line
continuation effectively commented out all the entries
after it.

Drop the commented out entry and note elsewhere
why it is commented out.

@garlick garlick force-pushed the garlick:event_api branch 2 times, most recently from 143b029 to 44e4325 Apr 25, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 25, 2018

Nice!

From the coverage, it appears that only the EINVAL case is covered for flux_event_publish_raw
(See codecov diff) It is a minor thing though.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

I'll see if I can fix that. I noted that I needed to add tests for publishing raw events and then promptly forgot!

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 25, 2018

I'll see if I can fix that. I noted that I needed to add tests for publishing raw events and then promptly forgot!

Oh, ugh, my fault for not reading to the end of the PR description. I totally missed that!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

I added some cleanup to the flux-event(1) command including a man page, and hopefully got the missing coverage by having the sharness event test publish some raw payload events with the command.

Note that the bindings may need some work to support raw event payloads (or raw payloads for other message types for that matter). I did note that lua f.event_recv() does insist on decoding payloads as JSON and we may run into problems with lua code such as synchronous_subscribe() in flux-aggreagate(1) once we have events with raw payloads in common circulation.

In that particular instance, it seems like a lua binding for a new flux_event_publish_seq() as discussed in #1486 could replace the function. However, I'm not sure whether there are similar issues elsewhere.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

Restarted one builder that hit #1077

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

Don't merge this - I am having second thoughts on the interface for flux_event_publish() et al...

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 25, 2018

Note that the bindings may need some work to support raw event payloads (or raw payloads for other message types for that matter). I did note that lua f.event_recv() does insist on decoding payloads as JSON and we may run into problems with lua code such as synchronous_subscribe() in flux-aggreagate(1) once we have events with raw payloads in common circulation.

I'm not sure what kind of support raw payloads could get from higher level bindings in general. Any service that encodes messages or events with raw payloads would probably need to be specifically wrapped for the bindings in order to have a hope of accessing the payload.

In the case of synchronous_subscribe() the event_recv() is done on an event generated by the script itself so I'm not sure how that event could have anything but a json payload. (which makes me think I don't fully understand the issue here)

That being said the bindings do need to all be regenerated anyway, so at that time we could revisit the need for interfaces to the raw msg types.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

Oh right, the subscription filter would prevent that. Sorry!

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 25, 2018

I think what I want to is pare this PR back to the new encode/decode functions only, and rework the flux_event_publish() interface after some discussion in #1486.

I can probably split out the flux-event(1) cleanup and man page to a separate PR as well.

@garlick garlick force-pushed the garlick:event_api branch from 3fa38f5 to 9f6278f Apr 25, 2018

@garlick garlick changed the title libflux: add missing event API functions libflux: add flux_event_encode_raw(), flux_event_decode_raw() Apr 26, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 26, 2018

@garlick, sorry it is difficult to tell -- has this PR been pared down as much as you wanted and is ready to merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 26, 2018

Yeah, it could go in. Sorry I should have commented.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 26, 2018

Ok, we'll merge this and then your other event PR, then rebase #1481 again.

@grondo grondo merged commit 2611b9f into flux-framework:master Apr 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.983%
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 26, 2018

Thank you!

@garlick garlick deleted the garlick:event_api branch Apr 26, 2018

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.