[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

This patch changes the logging init sequence to install a wrapper around
the built-in glog Loggers. The wrapper starts a new thread which does
double-buffering: log entries are buffered in a vector, and the thread
is woken up. The thread swaps in a clean buffer to avoid delaying
application threads while it flushes the original buffer to disk.

It's hard to test the end-to-end integration in a unit test, since the
unit tests disable logging to files, and this path only affects
file-based logging. However, I ran an earlier version of this patch in a
stress test environment and it seemed to reduce the frequency with which
I saw threads blocked on glog.

The patch does, however, have a test which exercises the new code paths,
including the blocking path. I looped the new test 500 times in TSAN
mode with success.

The new feature is enabled by default, but I left a hidden flag to
disable it in case we have any issues.

Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Reviewed-on: http://gerrit.cloudera.org:8080/5321
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_logger.cc
A src/kudu/util/async_logger.h
M src/kudu/util/debug/leakcheck_disabler.h
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
6 files changed, 461 insertions(+), 2 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 7: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5321

to look at the new patch set (#7).

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..

KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

This patch changes the logging init sequence to install a wrapper around
the built-in glog Loggers. The wrapper starts a new thread which does
double-buffering: log entries are buffered in a vector, and the thread
is woken up. The thread swaps in a clean buffer to avoid delaying
application threads while it flushes the original buffer to disk.

It's hard to test the end-to-end integration in a unit test, since the
unit tests disable logging to files, and this path only affects
file-based logging. However, I ran an earlier version of this patch in a
stress test environment and it seemed to reduce the frequency with which
I saw threads blocked on glog.

The patch does, however, have a test which exercises the new code paths,
including the blocking path. I looped the new test 500 times in TSAN
mode with success.

The new feature is enabled by default, but I left a hidden flag to
disable it in case we have any issues.

Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_logger.cc
A src/kudu/util/async_logger.h
M src/kudu/util/debug/leakcheck_disabler.h
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
6 files changed, 461 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/5321/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

PS4, Line 69: ull(*active
> > BTW, Kudu should never Write() FATAL messages given that we aren't wrappi
Ah, I forgot about that.

Could you update the class doc to describe the flush semantics of glog in a 
little more detail, as I suggested above?


http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.h
File src/kudu/util/async_logger.h:

PS4, Line 128:   size = 0;
 :   flush = false;
> Sure, or 'needs_flush_or_write'.
Were you going to rename this?


-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5321

to look at the new patch set (#6).

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..

KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

This patch changes the logging init sequence to install a wrapper around
the built-in glog Loggers. The wrapper starts a new thread which does
double-buffering: log entries are buffered in a vector, and the thread
is woken up. The thread swaps in a clean buffer to avoid delaying
application threads while it flushes the original buffer to disk.

It's hard to test the end-to-end integration in a unit test, since the
unit tests disable logging to files, and this path only affects
file-based logging. However, I ran an earlier version of this patch in a
stress test environment and it seemed to reduce the frequency with which
I saw threads blocked on glog.

The patch does, however, have a test which exercises the new code paths,
including the blocking path. I looped the new test 500 times in TSAN
mode with success.

The new feature is enabled by default, but I left a hidden flag to
disable it in case we have any issues.

Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_logger.cc
A src/kudu/util/async_logger.h
M src/kudu/util/debug/leakcheck_disabler.h
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
6 files changed, 453 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/5321/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5321/5/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

Line 88: l.Unlock();
> Nit: you could move the Unlock() above the conditional. Maybe adjusting the
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

PS4, Line 69: force_flush
> Makes sense; I didn't really understand what force_flush=true meant so I as
> BTW, Kudu should never Write() FATAL messages given that we aren't wrapping 
> the FATAL-level logger, but defensively programming for AsyncLogger is a good 
> thing.

Well, even though we don't wrap the FATAL destination, every FATAL message also 
gets written to the other log files (INFO, WARN, and ERROR) first. So, this 
actually has a positive effect (I verified without this code that the last 
INFO/WARNING messages could sometimes get chopped).


-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

PS4, Line 69: l(*active_b
> That would defeat a lot of the perf benefit here. It's worth noting that wh
Makes sense; I didn't really understand what force_flush=true meant so I 
assumed it was the same as Flush().

In that same class doc (or in comments above Write() and Flush() it would be 
helpful if you explicitly documented the glog flush semantics of Write(false, 
...), Write(true, ...), and Flush() respectively.

BTW, Kudu should never Write() FATAL messages given that we aren't wrapping the 
FATAL-level logger, but defensively programming for AsyncLogger is a good thing.


http://gerrit.cloudera.org:8080/#/c/5321/5/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

Line 88: l.Unlock();
Nit: you could move the Unlock() above the conditional. Maybe adjusting the 
scope of l would be better.


http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.h
File src/kudu/util/async_logger.h:

PS4, Line 128:   size = 0;
 :   flush = false;
> yes, that's the meaning of it. Suggest a better name? 'needs_action'?
Sure, or 'needs_flush_or_write'.


Line 140: bool empty() const {
> I wanted to make this code general purpose since the same issue is affectin
Hmm, OK. It's worth noting that Impala has a thread implementation of its own 
with an API very similar to ours, as we ported theirs into Kudu.

But your point about logging during thread startup is valid.


PS4, Line 146:   };
 : 
> yea, no time for that now though
If you think any of these proposed metrics are interesting, perhaps you can 
file a newbie JIRA to be fixed later?


-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5321

to look at the new patch set (#5).

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..

KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

This patch changes the logging init sequence to install a wrapper around
the built-in glog Loggers. The wrapper starts a new thread which does
double-buffering: log entries are buffered in a vector, and the thread
is woken up. The thread swaps in a clean buffer to avoid delaying
application threads while it flushes the original buffer to disk.

It's hard to test the end-to-end integration in a unit test, since the
unit tests disable logging to files, and this path only affects
file-based logging. However, I ran an earlier version of this patch in a
stress test environment and it seemed to reduce the frequency with which
I saw threads blocked on glog.

The patch does, however, have a test which exercises the new code paths,
including the blocking path. I looped the new test 500 times in TSAN
mode with success.

The new feature is enabled by default, but I left a hidden flag to
disable it in case we have any issues.

Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_logger.cc
A src/kudu/util/async_logger.h
M src/kudu/util/debug/leakcheck_disabler.h
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
6 files changed, 454 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/5321/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 4:

(32 comments)

http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

Line 20: #include "kudu/util/locks.h"
> Nit: should come after the system-level includes.
Done


PS4, Line 39: AsyncLogger::~AsyncLogger() {}
> nit: may be, just remove it and use
I've been told in the past it's better to define destructors explicitly in the 
.cc file, because otherwise the compiler has to generate them in every file 
that includes the header and then the linker has to de-duplicate them.


Line 42:   CHECK_EQ(state_, INITTED);
> Nit: I believe we follow an "expected value before actual value" convention
I think that's true only for ASSERT. Looking at 'git grep CHECK.*state' it's 
all in this order.


Line 49: CHECK_EQ(state_, RUNNING);
> These could be DCHECKs instead?
keeping as CHECK since these aren't hot paths. Fixing the issue you mentioned 
by making Start() set the state to RUNNING instead of the thread itself.


Line 53:   thread_.join();
> Does this throw if you call Stop() without first calling Start()? If so, co
the CHECK above would fire


Line 63:   DCHECK_EQ(state_, RUNNING);
> Is the expectation that there are no threads inside Write() or Flush() when
Yes, I assume that Stop() is only called after ensuring that no threads could 
be logging anymore (hence the CHECKs).

I guess the short circuit in Flush() is probably not entirely necessary, but as 
is, it allows you to call Flush in one thread, and then while it's still 
'waiting' for the flush, to stop.

In practice, I'm not super concerned because in real life we only install this 
AsyncLogger thing once at the beginning of the program, and never stop it. Stop 
is really there just for the purposes of tests. I'll make a note of that in the 
docs.


Line 65: app_threads_blocked_count_++;
> Depending on whether you think our condition variable implementation is vul
eh, it's just for tests anyway. will name it as such.


PS4, Line 69: force_flush
> If this is 'true', will this Write() call be followed up by a Flush() call?
That would defeat a lot of the perf benefit here. It's worth noting that when 
glog sets 'force flush', it still doesn't guarantee durability (fsync), it just 
tries to get it written "a bit faster" by flushing the app buffers to the OS 
file.

I suppose the semantics are slightly weaker here than what is provided by glog 
itself, in that glog will make sure it makes it to the OS on "force flush" 
writes, but this async logger won't. Really IMO this only matters for FATAL 
messages or messages just prior to FATAL. Given that, I changed the code to do 
what you suggested and call through to Flush() when we see a fatal. I also 
verified that, when I run a kudu-tserver misconfigured such that it FATALs, I 
see all of the appropriate log entries in the file.

I'll also make a note in the class doc about the weaker semantics.


Line 70:   wake_flusher_cond_.Signal();
> In this implementation, adding just one message will wake up RunThread() an
I don't want to add extra complexity here -- postponing the Signal would make 
the code a lot more complicated and I don't think the cost of the context 
switch is that bad here. Empirically this already helps a ton vs the default 
implementation, and if we really cared about context switch costs etc we'd go 
with a lock-free queue or somesuch.


Line 79:   auto orig_flush_count = flush_count_;
> Odd use of 'auto' given flush_count_ is just a uint64_t.
Done


Line 97:   wake_flusher_cond_.Wait();
> Suppose we have a completely idle (as in, nothing being glogged) T1 that's 
Done


PS4, Line 128: swap
> nit: would std::swap() work for AsyncLogger::Buffer as is?
nope, because that relies on Buffer to have a copy constructor or move 
constructor


http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.h
File src/kudu/util/async_logger.h:

PS4, Line 21: #include 
: #include 
: #include 
> Nit: resort (strange clang-tidy didn't flag this).
Done


PS4, Line 73:   // Return a count of how many times an application thread was
:   // blocked due to the buffers being full and the writer thread
:   // not keeping up.
> This sounds like something worth exposing as a metric. Maybe even exclusive
I changed it to be labeled as a test method. Didn't want to do the plumbing to 
make it a metric now.


Line 82:   struct Buffer;
> Seems like you could avoid this forward declaration if you reordered this s
Done


Line 121: void add(Msg msg, bool flush) {
> Nit: separate these functions with empty lines.
Done


PS4, Line 124: |=
> Is this bitwise operator safe for booleans?
yea, there is no ||= operator, this is the recommended one


PS4, Line 128: // A buffer is 

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.cc
File src/kudu/util/async_logger.cc:

PS4, Line 39: AsyncLogger::~AsyncLogger() {}
nit: may be, just remove it and use

~AsyncLogger() = default;

in the header?


PS4, Line 128: swap
nit: would std::swap() work for AsyncLogger::Buffer as is?


http://gerrit.cloudera.org:8080/#/c/5321/4/src/kudu/util/async_logger.h
File src/kudu/util/async_logger.h:

PS4, Line 124: |=
Is this bitwise operator safe for booleans?


PS4, Line 134: DISALLOW_COPY_AND_ASSIGN(Buffer);
I think for this to do what's is intended to do, it should be in the 'private' 
section, not 'public' one.


-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5321

to look at the new patch set (#3).

Change subject: KUDU-695. Avoid glog contention by deferring log writes to 
dedicated threads
..

KUDU-695. Avoid glog contention by deferring log writes to dedicated threads

This patch changes the logging init sequence to install a wrapper around
the built-in glog Loggers. The wrapper starts a new thread which does
double-buffering: log entries are buffered in a vector, and the thread
is woken up. The thread swaps in a clean buffer to avoid delaying
application threads while it flushes the original buffer to disk.

It's hard to test the end-to-end integration in a unit test, since the
unit tests disable logging to files, and this path only affects
file-based logging. However, I ran an earlier version of this patch in a
stress test environment and it seemed to reduce the frequency with which
I saw threads blocked on glog.

The patch does, however, have a test which exercises the new code paths,
including the blocking path. I looped the new test 500 times in TSAN
mode with success.

The new feature is enabled by default, but I left a hidden flag to
disable it in case we have any issues.

Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_logger.cc
A src/kudu/util/async_logger.h
M src/kudu/util/debug/leakcheck_disabler.h
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
6 files changed, 419 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/5321/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5321
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie22a0a29fa00a988a53a15d2c726ce5d49018f4d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon