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-ping refactor and cleanup #898

Merged
merged 8 commits into from Nov 21, 2016

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Nov 9, 2016

Per discussion in PR #860 and some offline discussion, cleanup flux-ping to use jansson API. I'm not 100% done as I noticed a few things, but thought I'd start w/ this PR.

Lots of mini-cleanup. Notable changes:

For consistency to ping(8), change --delay to --interval in flux-ping. This is an outright breakage of old command line but thought important to make it consistent and now's the time to do it. acc62ef

Refactor flux_rpc_multi and add a flux_rpcf_multi jansson equivalent. Stuck typdef of callback in the middle of the file, right before usage. It's not the style I normally use, but noticed it elsewhere. Could cleanup usage all around if not liked. 9617183 & 7cab3c6

Use jansson API in flux-ping. Not as cleaned up as I hoped. It's a complete wash when it comes to LOC. Perhaps need to just call flux_rpcf_multi regardless if it's a single or multi-call. Haven't quite figured out the right logic for that yet. Perhaps a rank of "any" could be another key that can be accepted by the flux_rpc_multi functions? 87ee115

I continue to use int64_t where it would be preferable to use json_int_t, but there's some header collisions between libjson-c and jansson. Can't remove shortjson.h header, because it's needed for tstat.h. So should refactor that somehow.

Another thing I noticed is that in message.c we got this check for json_int_t. Now that json_int_t can be used multiple places, I'm thinking we should stick it in configure and just fail if longs aren't 8 bytes.

/* Ensure %I format for jansson pack/unpack works with int64_t
 */
#if JSON_INTEGER_IS_LONG_LONG
# if SIZEOF_LONG_LONG != 8
# error json_int_t is not 64 bits
# endif
#else
# if SIZEOF_LONG != 8
# error json_int_t is not 64 bits
# endif
#endif

@garlick garlick added the review label Nov 9, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 9, 2016

Coverage Status

Coverage increased (+0.07%) to 76.016% when pulling 87ee115 on chu11:fluxpingcleanup into da42f80 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 9, 2016

Current coverage is 72.53% (diff: 80.00%)

Merging #898 into master will decrease coverage by <.01%

@@             master       #898   diff @@
==========================================
  Files           159        159          
  Lines         27173      27162    -11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19711      19702     -9   
+ Misses         7462       7460     -2   
  Partials          0          0          
Diff Coverage File Path
•••••••• 80% src/cmd/flux-ping.c

Powered by Codecov. Last update 3defa97...366409d

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 9, 2016

Couple quick comments:

Can't remove shortjson.h header, because it's needed for tstat.h. So should refactor that somehow.

My first thought would be to drop tstat_json() from tstat.[ch]. Since the tstat_t struct is exposed, the few places that need a JSON encoder can just do it themselves using whatever is convenient in that context.

Another thing I noticed is that in message.c we got this check for json_int_t. Now that json_int_t can be used multiple places, I'm thinking we should stick it in configure and just fail if longs aren't 8 bytes.

That sounds fine with me. The idea was that int64_t would be used in place of json_int_t in Flux API calls (and I think the man pages document it that way).

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 9, 2016

That sounds fine with me. The idea was that int64_t would be used in place of json_int_t in Flux API calls (and I think the man pages document it that way).

Ahh, had missed that it had been documented with int64_t. Reminds me I need to add the manpages for flux_rpcf_multi too.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 9, 2016

The flux_rpcf_multi() control flow is a bit hard to follow (internally in rpc.c) with the introduction of the callbacks. I'm wondering if the refactoring was worth it or if it would be clearer to replicate the shared code, or abstract some of it to a single shared function? I haven't worked through it in detail so maybe where you ended up is the best way. I find it confusing though. What's your opinion?

By the way IMHO it is fine to put callback typedefs at the top. I don't personally have a strong opinion on that one, or maybe would lean towards doing it at the top also.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 9, 2016

The refactor and flux_rpcf_multi() sort of came about due to not wanting to duplicate large amounts of the code and wanting to handle the cleanup of va_list neatly. Here's a pseudo-code of the logic when va_list was added into the middle of the original flux_rpc_multi():

flux_rpcf_multi(const char *fmt, ...)
{
    // bunch of setup including setting up w/ va_start()
    va_start (ap, fmt);
    for (i = 0; i < count; i++) {
        va_copy (ap_cpy, ap);
        if (rpc_send (fmt, ap_cpy) < 0) {
            va_end (ap_cpy);
            goto error;
         }
         va_end (ap_cpy);
    }
    // various cleanup, including gotta call va_end()
    va_end (ap);
    return rpc;
error:
    // cleanup and destroy everything, including va_end()
    va_end (ap);
    return NULL;
}

The va_* calls are sort of all over the place. The callbacks were to try and isolate all the va_* calls into a smaller snippet. Fully acknowledged maybe it's not worth it. We're certainly not at a "rule of three" situation, so perhaps an outright duplicate of the shared code would still be ok. I could really go either way, but if you've already find it difficult to grok, then perhaps code duplication would be better.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 10, 2016

Is there a way to avoid the repeated creation of the json payload and the varargs copying by creating the message once and then in the loop, calling rpc_request_prepare() to set the nodeid/matchtag, and flux_send(), then destroy the message outside the loop?

I wonder if that would refactor a little easier?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 10, 2016

Ahh, that's a good idea. Actually, at worst I can just build the json-str myself and just pass it directly into flux_rpc_multi. That'd be way better.

chu11 added some commits Nov 9, 2016

test/ping: Add new output format tests
Add new tests to check for proper formatting of ping output
given various combinations of input options.  Most notably, check
that output for single rank vs multiple rank ping works properly.
cmd/flux-ping: Fix resource leak corner case
flux_rpc_destroy() must be called regardless if there is data
to output.
cmd/flux-ping: Refactor with a rank_count
Add a rank_count variable internally to make code logic more
clear and correct.
cmd/flux-ping: Refactor for simpler logic
Take advantage of flux_rpcf_multi(3) "any" nodeset option,
eliminating need to use flux_rpcf(3) and the internal nodeid
variable.
test/ping: Add new tests
Add tests for "any" and "upstream" input options
cmd/flux-ping: Refactor to use liboptparse
In addition, remove unnecessary initializations now that
liboptparse is used.
doc/flux-ping(1): Document special case ranks
Document special case rank inputs of "any" and "upstream"

@chu11 chu11 changed the title flux-ping cleanup, update use to jansson API, follow up updates to flux API flux-ping refactor and cleanup Nov 20, 2016

@chu11 chu11 force-pushed the chu11:fluxpingcleanup branch from 87ee115 to 366409d Nov 21, 2016

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 21, 2016

After four spinoff PRs, (#902, #904, #909, #911) here is final flux-ping cleanup PR.

Discounting liboptparse cleanup, the LOC are largely unchanged, but I think the code is easier to follow w/ the jansson changes and the rank_count variable (and I believe it fixes some corner cases as well).

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage increased (+0.001%) to 76.132% when pulling 366409d on chu11:fluxpingcleanup into 3defa97 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 21, 2016

Nice! Thanks very much for working through this cleanup, and especially for the test cases and well factored PR's, which I know are a pain to work on but in the long run are well worth it IMHO.

@garlick garlick merged commit 55c99a3 into flux-framework:master Nov 21, 2016

4 checks passed

codecov/patch 80.00% of diff hit (target 72.53%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +7.46% compared to 3defa97
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 76.132%
Details

@garlick garlick removed the review label Nov 21, 2016

@grondo grondo referenced this pull request Nov 28, 2016

Closed

Create 0.6.0 release notes #916

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.