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

modules/kvs: Log to flux not stderr #1190

Merged
merged 1 commit into from Sep 15, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Sep 14, 2017

Replace lingering log calls that log to stderr with calls that
log to flux logging facility. Requires passing optional flux_t
handle to internal lookup and commit APIs.

Remove lingering unnecessary includes of log.h throughout.

Fixes #1165

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 14, 2017

Codecov Report

Merging #1190 into master will increase coverage by 0.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
+ Coverage   77.82%   77.83%   +0.01%     
==========================================
  Files         158      158              
  Lines       29288    29295       +7     
==========================================
+ Hits        22794    22803       +9     
+ Misses       6494     6492       -2
Impacted Files Coverage Δ
src/modules/kvs/kvs_util.c 87.75% <ø> (ø) ⬆️
src/modules/kvs/kvs.c 63.43% <100%> (+0.26%) ⬆️
src/modules/kvs/lookup.c 84.19% <55.55%> (-0.5%) ⬇️
src/modules/kvs/commit.c 77.41% <60%> (+0.06%) ⬆️
src/common/libkvs/kvs_commit.c 86.66% <0%> (-6.67%) ⬇️
src/common/libflux/content.c 86.66% <0%> (-3.34%) ⬇️
src/modules/connector-local/local.c 74.27% <0%> (-1.28%) ⬇️
src/common/libflux/mrpc.c 85.15% <0%> (-1.18%) ⬇️
src/broker/modservice.c 79.61% <0%> (-0.98%) ⬇️
src/common/libflux/rpc.c 93.38% <0%> (-0.83%) ⬇️
... and 17 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2017

Coverage Status

Coverage decreased (-0.01%) to 78.147% when pulling 0555fc8 on chu11:issue1165 into 8443eff on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Sep 14, 2017

codecov diff is unsurprisingly bad, as all changes are "impossible" to hit error cases

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 14, 2017

Yeah, I think we could ignore the coverage here.

I wonder if it would be a good idea to submit a mini-PR for the logging code before this one containing a change like this, so we can avoid the test for (h != NULL) at each log call?

diff --git a/src/common/libflux/flog.c b/src/common/libflux/flog.c
index 8eae5cb..9944464 100644
--- a/src/common/libflux/flog.c
+++ b/src/common/libflux/flog.c
@@ -134,7 +134,7 @@ done:
 
 int flux_vlog (flux_t *h, int level, const char *fmt, va_list ap)
 {
-    logctx_t *ctx = getctx (h);
+    logctx_t *ctx;
     int saved_errno = errno;
     uint32_t rank;
     int len;
@@ -143,7 +143,11 @@ int flux_vlog (flux_t *h, int level, const char *fmt, va_list ap)
     struct stdlog_header hdr;
     int rpc_flags = FLUX_RPC_NORESPONSE;
 
-    if (!ctx) {
+    if (!h) {
+        return vfprintf (stderr, fmt, ap);
+    }
+
+    if (!(ctx = getctx (h))) {
         errno = ENOMEM;
         goto fatal;
     }

I guess probably there'd be a bit more to it - man page update, a test, maybe include level in the output, interpreted.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Sep 14, 2017

@garlick I hadn't thought about that, but I think it's a good idea. I'll take a look at that first.

@chu11 chu11 force-pushed the chu11:issue1165 branch from 0555fc8 to ff0993a Sep 15, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Sep 15, 2017

re-pushed w/ updates given #1192

@@ -96,8 +97,10 @@ void commit_cleanup_dirty_cache_entry (commit_t *c, struct cache_entry *hp);
* commit_mgr_t API
*/

/* flux_t is optional, if NULL no logging will occur */

This comment has been minimized.

@garlick

garlick Sep 15, 2017

Member

might want to update this comment

This comment has been minimized.

@chu11

chu11 Sep 15, 2017

Author Contributor

oops! thanks

modules/kvs: Log to flux not stderr
Replace lingering log calls that log to stderr with calls that
log to flux logging facility.  Requires passing optional flux_t
handle to internal lookup and commit APIs.

Remove lingering unnecessary includes of log.h throughout.

Fixes #1165

@chu11 chu11 force-pushed the chu11:issue1165 branch from ff0993a to 2be2edd Sep 15, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 15, 2017

Coverage Status

Coverage increased (+0.007%) to 78.184% when pulling 2be2edd on chu11:issue1165 into 3cd405f on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 15, 2017

Thanks @chu11, this looks good.

@garlick garlick merged commit e570bd7 into flux-framework:master Sep 15, 2017

3 of 4 checks passed

codecov/patch 60% of diff hit (target 77.82%)
Details
codecov/project 77.83% (+0.01%) compared to 3cd405f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 78.184%
Details

@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.