Conversation
| public void close() throws SecurityException { | ||
| super.close(); | ||
| if (handlers.decrementAndGet() <= 0 ) { | ||
| queue.offer(null); |
There was a problem hiding this comment.
What's the purpose of this? Wouldn't it just cause a NullPointerException?
There was a problem hiding this comment.
offer(null) is just a poison pill for the LoggerThread
There was a problem hiding this comment.
It doesn't look like BlockingQueue allows adding null at all:
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/BlockingQueue.html#offer(E)
There was a problem hiding this comment.
Ah so much for testing! Anyway, null is a poor poison pill! I'll make a specific LogEvent instance that is the poison pill.
| @Override | ||
| public void publish(LogRecord record) { | ||
| if (isLoggable(record)) { | ||
| queue.offer(new LogEntry(record)); |
There was a problem hiding this comment.
Should the return value of offer() be checked?
There was a problem hiding this comment.
a return of false from .offer() means the queue is full.
knowing that, what would you do in that situation? (how would you handle false?)
There was a problem hiding this comment.
Currently I have gone with an unlimited log queue. If you look at the Juli example linked to, you will see that they have a fixed sized queue with configurable actions to do if the queue is full (discard, block etc.).
My thoughts on this is that any app that is suffering OOM issues because of a large logevent queue is probably going to have similar issues anyway, even if the logevents are discarded. I think an unlimited queue is thus the simplest and does not need configuration and/or tuning.
So if the unlimited policy is accepted, the offer return does not need to be checked. However I'm open to be convinced that another policy may be better?
| super.publish(record); | ||
| } | ||
|
|
||
| private class LogEntry { |
There was a problem hiding this comment.
What's the reason for introducing this class? Can the queue simply hold LogRecords directly?
There was a problem hiding this comment.
The LogEntry class is a nest class and thus holds a reference back to the handler instance that created it. Multiple handler instances can exist and they share the queue and thread. The async thread when doing the actual logging calls back to the handler instance.
This design is copied from the Juli log. It may not be necessary as we "know" that there should only be a single instance of the handler.
The other option is to have a none static queue and thread, so that every instance of the handler would create their own queue. This would not be an issue as we only create 1, but would at least work if more were created.
| assertThat(data.severity, equalTo("ERROR")); | ||
| assertThat( | ||
| data.message, org.hamcrest.Matchers.containsString("LoggingServlet doGet: not null")); | ||
| data.message, org.hamcrest.Matchers.containsString("com.foo.bar: not null")); |
There was a problem hiding this comment.
What accounts for this change?
There was a problem hiding this comment.
Very good question. I don't actually know how that test was passing before because the logs are explicitly generated from a "com.foo.bar" logger.
So I'll take this question on notice and research how it was working prior to this PR.
There was a problem hiding this comment.
The old jetty9-compat logging didn't log with the logger name, it used "<calling class> <calling method name>: <message>" (I know, its strange, but that's what they did)
|
@meltsufin I have pushed a simplification of the async logging. However I note that the JsonFormatter is using the current thread to set the thread field in the log record. This is now always the async logging thread. Thus we either need to not bother logging the thread, or add a mechanism to capture the logging thread and use that (may need a LogEvent class again). Your thoughts? |
|
Ah the LogRecord already has the integer thread ID, so the JsonFormatter should just use that rather than the current thread. |
|
Thread name is not quiet the same as thread id, and can also be useful in debugging. However, looking at the Log Viewer, it doesn't seem to be parsed out from the JSON anyway. So, I'm tempted to just remove it. |
|
@meltsufin note that for compat, the log is still created as a file on disk, so having some thread identifier there may still be useful in debugging, as at least it can be found. Thus I'm inclined to leave as is. I'll review how thread is being treated in the jetty-runtime stuff, but I think it is fine as it is only the write that is happening async, not the format. |
|
Sounds good. LGTM |
3 participants
No description provided.