[BEAM-1347] Remove the usage of a thread local on a potentially hot path#3260
[BEAM-1347] Remove the usage of a thread local on a potentially hot path#3260lukecwik wants to merge 1 commit into
Conversation
|
R: @bjchambers |
| private final Future<?> bufferedLogWriter; | ||
| private final ThreadLocal<Consumer<BeamFnApi.LogEntry>> logEntryHandler; | ||
| /** | ||
| * Safe object publishing is not required since we only care if the thread that set |
There was a problem hiding this comment.
Can you elaborate on the logic here? As far as I can tell, the goal is that the log publishing thread (which is responsible for draining the queue) will only ever offer the message, while other threads will attempt to wait. So we don't need a thread local, we just need to know if this is the thread responsible for draining the queue.
There was a problem hiding this comment.
This is more of a comment about not needing the volatile key word since the thread that sets it is the only thread that cares that it was set. All other threads compute the same result if it is null. Also, I believe it is adequately explained in the comments surrounding usage/setting in the code.
| if (Thread.currentThread() != logEntryHandlerThread) { | ||
| // Blocks caller till enough space exists to publish this log entry. | ||
| try { | ||
| bufferedLogEntries.put(builder.build()); |
There was a problem hiding this comment.
Should this have a time limit and noisily complain if the queue isn't drained in a reasonable amount of time? I'm thinking about cases where the logging thread mysteriously dies, it would be good to say "unable to put log entry after N minutes".
There was a problem hiding this comment.
Noisily complaining should be part of the container health check or logged to the docker container log. When those exist, this is an excellent place to wire logging as being an issue/bottleneck.
|
--none-- |
3 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify.<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.