Mike Percy has posted comments on this change. Change subject: KUDU-1767. c++ client: Prevent concurrent flushes by default ......................................................................
Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/5533/2/src/kudu/client/session-internal.cc File src/kudu/client/session-internal.cc: PS2, Line 82: condition_.Signal(); > I think the mutex_ should be held while signalling on the condition, otherw Thx for the review Alexey. I don't see any way for events to be missed by the waiters because only the thread calling Apply() can be waiting, which is the only thread that could race to modify the values of the variables we are waiting on. I looked into this a bit and below is my rationale. The GetBatchersToFlushUnlocked() function doesn't affect any variables waited on by the condition waiters (which are buffer_bytes_used_ and batchers_num_), so calling that method while we still have the lock allows us to avoid multiple lock acquisitions. I thought it was more readable to release the lock, immediately start the flush threads related to 'to_flush', and then put in the big block comment about the condition variable and signal it. Signaling the condition variable outside of the lock in this case doesn't seem to have any effect either way. In terms of correctness, according to the man page, it isn't required to be the holder of the lock while signaling: "The pthread_cond_broadcast() or pthread_cond_signal() functions may be called by a thread whether or not it currently owns the mutex that threads calling pthread_cond_wait() or pthread_cond_timedwait() have associated with the condition variable during their waits; however, if predictable scheduling behavior is required, then that mutex shall be locked by the thread calling pthread_cond_broadcast() or pthread_cond_signal()." To parse that out further, according to one of the authors of the pthreads standard @ https://groups.google.com/forum/#!msg/comp.programming.threads/wEUgPq541v8/ZByyyS8acqMJ the "predictable scheduling behavior" is mainly relevant in the context of realtime threads that have strict priority scheduling requirements. -- To view, visit http://gerrit.cloudera.org:8080/5533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3e48a054093163824573962963ddcba122a948b8 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
