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

libioencode: convenience library for encoding io #2293

Merged
merged 4 commits into from Aug 9, 2019

Conversation

@chu11
Copy link
Contributor

commented Aug 7, 2019

This simple convenience library encodes/decodes io into json objects. Being the non-creative person I am, I call the library libioencode. The objects encode stream, rank, data in base64, or EOF.

Cleanup was nice, to remove alot of the random calls to libsodium all over the place and to common-ize code between libsubprocess and shell.

Design choice: When encoding I elected to have a different object for EOF. When decoding, you can get data or EOF, but not both. I felt this made things simpler in libsubprocess, but can understand there are circumstances this is not optimal.

@garlick

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

In flux-framework/rfc#192, @grondo said

We should allow data and eof to appear in the same event (not explicitly disallowed here), since this could reduce the number of required events, especially with batching.

Maybe that applies here too? Just make both data and eof optional keys and set them to empty string and false, respectively, if missing?

If batching/compression is imminent, maybe we should make "rank" a string that can be an idset?

Thoughts on adding an integer timestamp?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Maybe that applies here too? Just make both data and eof optional keys and set them to empty string and false, respectively, if missing?

Sounds good. I went back and forth on it.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Thoughts on adding an integer timestamp?

I keep thinking that adding a timestamp at the source is going to cause more skew than it is worth (I assume you mean floating point timestamp?). Since the IO message has to be reencoded into an eventlog entry anyway, and the libeventlog API adds a timestamp for you, it seems less necessary to send the timestamp over the wire.

I could be convinced otherwise, though.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

If batching/compression is imminent, maybe we should make "rank" a string that can be an idset?

yeah, we might as well make "rank" an idset string now, though with the current format lines would have to be identical to be batched (from different tasks).

@chu11 chu11 force-pushed the chu11:libioencode branch from ca281c9 to 0f7780e Aug 7, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

re-pushed, now data & eof can be specified in one RPC

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

If batching/compression is imminent, maybe we should make "rank" a string that can be an idset?

Per discussion this afternoon, I think this should be a refactoring for later. It isn't used at all now, so it's sort of inconvenient for all of the current code where all output comes from a single rank.

@garlick
garlick approved these changes Aug 9, 2019
Copy link
Member

left a comment

I left one comment but it's not especially a big deal.

I'm OK with this going in now as it's a nice refactoring.

We will want to revisit and possibly change function signatures when we add support for alternate encodings and I/O event logs as discussed in flux-framework/rfc#192. At that point we may want to promote some interfaces to be part of the public Flux API.

0,
0,
"{s:O}",
"io", o)))

This comment has been minimized.

Copy link
@garlick

garlick Aug 9, 2019

Member

Is the outer JSON object needed here? Maybe just _pack (..., "O", o); would be sufficient.

This comment has been minimized.

Copy link
@chu11

chu11 Aug 9, 2019

Author Contributor

ahhh, good catch. IIRC flux_rpc() requires payloads to be json objects. So I instinctively always create one. But in this case, the ioencode creates a json object, so it can go straight in.

chu11 added 4 commits Jul 29, 2019
Simple convenience library encodes/decodes io into json objects
for transmission.
Includes changes to flux-job, which used object format defined by shell.
@chu11 chu11 force-pushed the chu11:libioencode branch from 0f7780e to 043c819 Aug 9, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

rebased, re-pushed with suggested change from @garlick. Squashed since it was so simple.

@mergify mergify bot merged commit 7bdda9d into flux-framework:master Aug 9, 2019
2 checks passed
2 checks passed
Summary 1 rule matches
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@codecov-io

This comment has been minimized.

Copy link

commented Aug 9, 2019

Codecov Report

Merging #2293 into master will increase coverage by <.01%.
The diff coverage is 84.71%.

@@            Coverage Diff             @@
##           master    #2293      +/-   ##
==========================================
+ Coverage   80.88%   80.88%   +<.01%     
==========================================
  Files         213      214       +1     
  Lines       33553    33649      +96     
==========================================
+ Hits        27139    27217      +78     
- Misses       6414     6432      +18
Impacted Files Coverage Δ
src/shell/io.c 75.7% <72%> (-2.71%) ⬇️
src/common/libsubprocess/remote.c 70.63% <81.81%> (+1.03%) ⬆️
src/common/libsubprocess/server.c 71.42% <82.14%> (-0.29%) ⬇️
src/cmd/flux-job.c 85.32% <83.33%> (+0.09%) ⬆️
src/common/libioencode/ioencode.c 92.3% <92.3%> (ø)
src/modules/connector-local/local.c 73.55% <0%> (-0.87%) ⬇️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.