[
https://issues.apache.org/jira/browse/ZOOKEEPER-1400?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13263546#comment-13263546
]
Michi Mutsuzaki commented on ZOOKEEPER-1400:
--------------------------------------------
Hi Marshall,
Sorry for the late response. This is a great patch! Here are my comments:
* The documentation for {{zoo_set_log_callback()}} should make it clear that
the callback will be invoked by multiple threads and therefore it needs to be
thread-safe.
* We probably want to log something if {{get_format_log_buffer()}} returns
NULL, something similar to "format_log_message: Unable to allocate memory
buffer".
* Do you need to pass {{FORMAT_LOG_BUF_SIZE-1}} to {{snprintf()}}, or would
{{FORMAT_LOG_BUF_SIZE}} work? I thought {{snprintf()}} guarantees that the
output is null-terminated.
* This patch should be unit-tested (now that it's much easier to unit test the
logging code with the callback :)).
* It would be great if you can share how you are using the callback for your
logging on our wiki page. I'm pretty sure many people will find it very useful.
* {quote}
While refactoring this code, I removed the need for the double parens in all
the LOG macros as that wasn't necessary and didn't fit with my new approach.
{quote}
Cool!
Regarding Stephen's comment:
* Yes, please put per-connection logging on a separate jira.
* {quote}
This means there is now a limit on log size that wasn't there before. Have you
made sure this buffer can handle all possible log messages?
{quote}
The longest line I got from the unit test was less than 500 characters. I also
eyeballed all the LOG_* calls, and they all look much less than 4096
characters. It's difficult to be absolutely sure since the log message is not
fixed length, but I think it would be ok.
* {quote}
Maybe those should be reserved for a separate change just to keep things tidy?
{quote}
Yes, I generally prefer patches without cosmetic changes.
Thanks!
--Michi
> Allow logging via callback instead of raw FILE pointer
> ------------------------------------------------------
>
> Key: ZOOKEEPER-1400
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1400
> Project: ZooKeeper
> Issue Type: Improvement
> Components: c client
> Affects Versions: 3.4.2
> Environment: Linux
> Reporter: Marshall McMullen
> Fix For: 3.4.2
>
> Attachments: case-2739.patch
>
>
> The existing logging framework inside the C client uses a raw FILE*. Using a
> FILE* is very limiting and potentially dangerous. A safer alternative is to
> just provide a callback that the C client will call for each message. In our
> environment, we saw some really nasty issues with multiple threads all
> connecting to zookeeper via the C Client related to the use of a raw FILE*.
> Specifically, if the FILE * is closed and that file descriptor is reused by
> the kernel before the C client is notified then the C client will use it's
> static global logStream pointer for subsequent logging messages. That FILE*
> is now a loose cannon! In our environment, we saw zookeeper log messages
> ending up in other sockets and even in our core data path. Clearly this is
> dangerous. In our particular case, we'd omitted a call to
> zoo_set_log_stream(NULL) to notify C client that the FILE* has been closed.
> However, even with that bug fixed, there's still a race condition where log
> messages in flight may be sent before the C client is notified of the FILE
> closure, and the same problem can happen.
> Other issues we've seen involved multiple threads, wherein one would close
> the FILE*, and that's a global change that affects all threads connected
> within that process. That's a pretty nasty limitation as well.
> My proposed change is to allow setting a callback for log messages. A
> callback is used in preference to a raw FILE*. If no callback is set, then it
> will fallback to the existing FILE*. If that's not set, then it falls back to
> stderr as it always has.
> While refactoring this code, I removed the need for the double parens in all
> the LOG macros as that wasn't necessary and didn't fit with my new approach.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
