You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/12/16 02:43:31 UTC
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5533
to review the following change.
Change subject: KUDU-1767. c++ client: Prevent concurrent flushes by default
......................................................................
KUDU-1767. c++ client: Prevent concurrent flushes by default
This patch makes it so that only a configurable number of batchers can
flush simultaneously in the C++ client. The default is 1, making it so
that flushes occur serially, although applies may take place during an
outstanding flush as long as there are available buffers (the default
number of mutation buffers remains 2).
This patch also adds some basic unit testing for this functionality and
enables the previously-failing client op interleave test.
Change-Id: I3e48a054093163824573962963ddcba122a948b8
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/client_flush_interleave-itest.cc
6 files changed, 205 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/5533/1
--
To view, visit http://gerrit.cloudera.org:8080/5533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3e48a054093163824573962963ddcba122a948b8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo 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();
> Thx for the review Alexey. I don't see any way for events to be missed by t
I haven't reviewed the code but I wanted to add two points about this:
1. Almost all (but not all) of our ConditionVariable-using code holds the mutex during a Signal() or Broadcast().
2. While ConditionVariable is implemented with pthreads today, this could change in the future. There's nothing pthread-specific about the interface, so we shouldn't poke a hole in the abstraction by relying on any pthread-only semantics, in case the underlying implementation changes, or in case Kudu is ported to a platform without pthreads (e.g. Windows). I don't know if that's the case here; I just wanted to bring up the point.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has uploaded a new patch set (#2).
Change subject: KUDU-1767. c++ client: Prevent concurrent flushes by default
......................................................................
KUDU-1767. c++ client: Prevent concurrent flushes by default
This patch makes it so that only a configurable number of batchers can
flush simultaneously in the C++ client. The default is 1, making it so
that flushes occur serially, although applies may take place during an
outstanding flush as long as there are available buffers (the default
number of mutation buffers remains 2).
This patch also adds some basic unit testing for this functionality and
enables the previously-failing client op interleave test.
Change-Id: I3e48a054093163824573962963ddcba122a948b8
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/client_flush_interleave-itest.cc
6 files changed, 206 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/5533/2
--
To view, visit http://gerrit.cloudera.org:8080/5533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e48a054093163824573962963ddcba122a948b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change.
Change subject: KUDU-1767. c++ client: Prevent concurrent flushes by default
......................................................................
Abandoned
Abandoning this workaround. For many ingest use cases, where people consider the primary data dependency to be a key-based dependency, we could provide a mode to provide a kind of "eventual consistency" ingest model as an alternative to AUTO_BACKGROUND_FLUSH, which provides no ordering guarantees. I filed KUDU-1841 to track that idea.
--
To view, visit http://gerrit.cloudera.org:8080/5533
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3e48a054093163824573962963ddcba122a948b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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, otherwise those events might be missed by the thread waiting on that condition variable.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1767. c++ client: Prevent concurrent flushes by default
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1767. c++ client: Prevent concurrent flushes by default
......................................................................
Patch Set 2: Code-Review-1
I don't think this is a particularly good idea to "fix".
Making only a single flush outstanding at a time will regress performance substantially, and I don't think the improved semantics are worth it.
Although this avoids one particular case of the insertion order not matching the order rows are applied to the session, there are plenty of others:
- if I wrote to row A, and then to row B, and those are different tablets, B can be assigned a timestamp earlier than A.
- if I write row A, and it gets a timeout for its flush, and then write row B, row A's write may actually still succeed after row B's
- the typical use of auto-flush mode is bulk ingest, eg something like an MR job or Impala INSERT query. In that case, you'll almost certainly have tens or hundreds of parallel inserters, in which case you have no guarantee that two "sequential" rows in the insert data are actually picked up by the same inserter thread. For example consider an MR job, where the two versions of the same row key fall just across an HDFS block boundary. You have no ordering guarantee between them.
I think it's much better to just say "there are no ordering guarantees provided by auto_flush_background,but it has the best performance". If someone wants ordering, they need to explicitly wait on a row to be flushed before they may send the next dependent row, rather than make all use cases take a big perf hit.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No