You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/12/01 00:45:01 UTC

[kudu-CR] WIP: Make sure replica transactions start/abort on the consensus thread

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: WIP: Make sure replica transactions start/abort on the consensus thread
......................................................................

WIP: Make sure replica transactions start/abort on the consensus thread

In order for a replica to safely move "safe" time, it needs to know
that all transactions that come before it have at least started.
One way to do this is to call transaction_->Start() on
transaction_driver_->Init() which we know is called by the consensus
thread. This should be ok as the leader has already correctly serialized
the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done on the consensus thread) we
now must make sure that the transaction is actually removed from
mvcc in-line. Otherwise the consensus thread might call Start() on
another transaction with the same timestamp before the failed
transaction is removed from mvcc.

To do this, this patch add a latch to make sure that, if replication
failed, the prepare phase has completed.

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 36 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about and Prepare() might still
be acquiring row locks.

I ran the new raft_consensus-itest that includes unique/duplicate
key workloads and exactly once semantics in dist-test with asan,
slow mode and 1 stress thread. 3/1000 tests failed. This is inline
with the baseline from the patch that changed the itest.

Results:
http://dist-test.cloudera.org//job?job_id=david.alves.1480768100.17891

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
5 files changed, 60 insertions(+), 40 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/16
--
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about and Prepare() might still
be acquiring row locks.

I ran the new raft_consensus-itest that includes unique/duplicate
key workloads and exactly once semantics in dist-test with asan,
slow mode and 1 stress thread. 3/1000 tests failed. This is inline
with the baseline from the patch that changed the itest.

Results:
http://dist-test.cloudera.org//job?job_id=david.alves.1480811163.22558

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
6 files changed, 61 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/19
-- 
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................

KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

In order for a replica to safely move "safe" time, it needs to know
that all transactions that come before it have at least started.
One way to do this is to call transaction_->Start() on
transaction_driver_->Init() which we know is called by the consensus
thread. This should be ok as the leader has already correctly serialized
the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done on the consensus thread), we
now must make sure that the transaction is actually removed from
mvcc in-line. Otherwise the consensus thread might call Start() on
another transaction with the same timestamp before the failed
transaction is removed from mvcc.

To do this, this patch adds a latch to make sure that, if replication
failed, the prepare phase has completed.

Finally this patch adds a couple of statements to increase the
driver's refcount so that it survives until the latch may be called.

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 60 insertions(+), 25 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5294/4//COMMIT_MSG
Commit Message:

PS4, Line 12: which we know
> Not sure this is true, see comments inline
I meant "replica" transactions, will clarify


PS4, Line 18: consensus thread
> Is the consensus thread the prepare thread? Not sure which pool we are talk
by "consensus thread" I mean the thread that is updating consensus and that is triggering replica transactions. This doesn't change the "leader transactions" path in terms of Start()


http://gerrit.cloudera.org:8080/#/c/5294/3/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

PS3, Line 431:   if (PREDICT_FALSE(replication_state_copy == REPLICATION_FAILED)) {
             :     prepare_latch_.Wait();
             :   }
> at first glance this latch thing seems a bit sketchy, but I need to spend a
I tried a couple of different things before I resorted to the latch. The main point here is that we need to make sure that the transaction is no longer on mvcc before returning from this method.

Because of the race between the Prepare(), running on the prepare pool, and this method, running on the consensus update thread, to call ApplyAsync() it's hard to have this guarantee.

I tried different ways of fudging with the vars but ultimately could not find a non-racy way to make sure that, If we changed the state to REPLICATION_FAILED above in time for the prepare thread to see it (and move on to apply, which will actually abort), making sure that it completes before returning from this method.

Note that we only wait on the latch for replica transactions that have failed.


http://gerrit.cloudera.org:8080/#/c/5294/4/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 102:       prepare_latch_(0) {
> Since a given transaction prepares only once, why not initialize the prepar
we need to only set the latch if the transaction actually Init'ed want to be double sure that it's submitted to prepare before we risk waiting.


PS4, Line 118: in the consensus thread
> I'm confused by this comment. I'm pretty sure this is executed on a TabletS
I meant by the thread that is updating consensus. note that this is the "REPLICA" path. Will make this clearer in the comments.


PS4, Line 148: innited
> typo: inited
Done


Line 391:   scoped_refptr<TransactionDriver> ref(this);
> This is pretty smelly. Can we not ensure the caller has a ref, instead? Als
the object won't be destroyed before we change the state below.
I agree that lifetime of this class is screwed up, but we seem to resort to increasing the refcount in several other places and would like not to put sorting that out on the critical path.


Line 435: 
> nit: spurious line
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................

KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init() which we know is called by the thread
updating consensus, for non-leader transctions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we now must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this, this patch adds a latch to make sure that, if replication
failed, the prepare phase has completed before returning from
ReplicationFinished().

Finally this patch adds a couple of statements to increase the
driver's refcount so that it survives until the latch may be called.

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 61 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5294/6/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 148:   RETURN_NOT_OK(txn_tracker_->Add(this));
> if this fails, then it seems like transaction_->Start() would have been cal
you're right, I will add a test to make sure this doesn't happen. maybe even solve KUDU-625


PS6, Line 421: If replication has failed we need to make sure the transaction is removed from mvcc before we
             :   // return from this method,
> seems like it would be cleaner to just be using some kind of locking here..
yeah, that makes sense to me. I think this class is due for a new overall.


PS6, Line 429: replication_state_copy
> why not just read 'status' directly?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................


Patch Set 13:

(1 comment)

k, i like this approach more. BTW is there a later test in this series that validates that this does as it purports?

http://gerrit.cloudera.org:8080/#/c/5294/13/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

PS13, Line 381:   // Increase our ref count so that we're sure the driver survives until after we wait on the latch,
is this still needed?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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] WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about.

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
5 files changed, 61 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/13
-- 
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about and Prepare() might still
be acquiring row locks.

I ran the new raft_consensus-itest that includes unique/duplicate
key workloads and exactly once semantics in dist-test.

Results of slow mode, 1 stress thread, 1000 loop runs of
raft_consensus-itest on dist-test.

TSAN-prev (16/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480814757.31031
TSAN-this (10/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480812541.27015

I inspected the TSAN failures and they were mostly ~DnsResolver()
races on test code itself. The tests themselves passed for the
most part. Some ended up timing out with archives that are too
big to download. This also happens in the previous patch.

ASAN-prev (02/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480817418.1140
ASAN-this (03/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480811163.22558

I inspected the ASAN failures. Two of them are test only flakes.
One of them is worrying as the consensus queue remains full,
suggesting a deadlock, but this also happening on the previous
patch. Example:

https://kudu-test-results.s3.amazonaws.com/david.alves.1480817418.1140.36139843995986a54aaf9f533a00111f384e85a6.145.0-artifacts.zip?Signature=PjjGYVvmWns4t0M7j6%2FAZQYVj34%3D&Expires=1480904781&AWSAccessKeyId=AKIAJ2NR2VXMAHTVLMRA

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
5 files changed, 60 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................


Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5294/4//COMMIT_MSG
Commit Message:

PS4, Line 12: which we know
Not sure this is true, see comments inline


PS4, Line 18: consensus thread
Is the consensus thread the prepare thread? Not sure which pool we are talking about here.


http://gerrit.cloudera.org:8080/#/c/5294/4/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 102:       prepare_latch_(0) {
Since a given transaction prepares only once, why not initialize the prepare_latch_ to 1? Then on abort we can count it down.


PS4, Line 118: in the consensus thread
I'm confused by this comment. I'm pretty sure this is executed on a TabletService service pool thread:

TabletServiceImpl::Write() -> TabletPeer::SubmitWrite() -> TabletPeer::NewLeaderTransactionDriver() -> TransactionDriver::Init()


PS4, Line 148: innited
typo: inited


Line 391:   scoped_refptr<TransactionDriver> ref(this);
This is pretty smelly. Can we not ensure the caller has a ref, instead? Also, if this is so racy, what guarantees that this object is not destroyed before we take the ref on this line?

If we really must do this then we should do a more careful job of documenting the lifecycle guarantees.


Line 435: 
nit: spurious line


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5294/12/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 430:     prepare_latch_.Wait();
> thinking about this some more, I'm afraid of the following deadlock scenari
verified this is the case. posted a repro test case at http://gerrit.cloudera.org:8080/5341


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about and Prepare() might still
be acquiring row locks.

Results:
http://dist-test.cloudera.org//job?job_id=david.alves.1480768100.17891

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/17
--
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5294/6/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 148:   RETURN_NOT_OK(txn_tracker_->Add(this));
if this fails, then it seems like transaction_->Start() would have been called, and then we're relying on the destructor here to abort the mvcc transaction? is that path well covered? not sure if we have a test which makes a replica transaction tracker hit its memory limit, and KUDU-625 makes me nervous


PS6, Line 421: If replication has failed we need to make sure the transaction is removed from mvcc before we
             :   // return from this method,
seems like it would be cleaner to just be using some kind of locking here... I don't want to block progress on this if we have stress tests showing that it works OK, but let's think more about how to restructure the different prepare/replicate/etc interleavings to be more comprehensible.


PS6, Line 429: replication_state_copy
why not just read 'status' directly?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this, this patch adds a latch to make sure that, if replication
failed, the prepare phase has completed before returning from
ReplicationFinished().

Finally this patch adds a couple of statements to increase the
driver's refcount so that it survives until the latch may be called.

Ran this in dist-test, slow mode, asan, 1000 times. No failures.

Results:
exactly_once_writes-itest: http://dist-test.cloudera.org//job?job_id=david.alves.1480659005.9899
raft_consensus-itest     : http://dist-test.cloudera.org//job?job_id=david.alves.1480659414.10085

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 58 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................

KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init() which we know is called by the thread
updating consensus, for non-leader transctions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we now must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this, this patch adds a latch to make sure that, if replication
failed, the prepare phase has completed before returning from
ReplicationFinished().

Finally this patch adds a couple of statements to increase the
driver's refcount so that it survives until the latch may be called.

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 58 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about and Prepare() might still
be acquiring row locks.

I ran the new raft_consensus-itest that includes unique/duplicate
key workloads and exactly once semantics in dist-test with asan,
slow mode and 1 stress thread. 3/1000 tests failed. This is inline
with the baseline from the patch that changed the itest.

Results of slow mode, 1 stress thread 1000 loop runs of
raft_consensus-itest on dist-test:

TSAN-prev (16/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480814757.31031
TSAN-this (10/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480812541.27015

I inspected the TSAN failures and they were mostly ~DnsResolver()
races on test code itself. The tests themselves passed for the
most part. Some ended up timing out with archives that are too
big to download. This also happens in the previous patch.

ASAN-prev (02/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480817418.1140
ASAN-this (03/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480811163.22558

I inspected the ASAN failures. Two of them are test only flakes.
One of them is worrying as the consensus queue remains full,
suggesting a deadlock, but this also happening on the previous
patch. Example:

https://kudu-test-results.s3.amazonaws.com/david.alves.1480817418.1140.36139843995986a54aaf9f533a00111f384e85a6.145.0-artifacts.zip?Signature=PjjGYVvmWns4t0M7j6%2FAZQYVj34%3D&Expires=1480904781&AWSAccessKeyId=AKIAJ2NR2VXMAHTVLMRA

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
5 files changed, 60 insertions(+), 40 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/20
-- 
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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] WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................


Patch Set 12:

no, I'll clean this up and run the dist-tests again.
I'll also come up with an additional unit test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has submitted this change and it was merged.

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................


KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init(). We know Init() is called by the thread
updating consensus, for non-leader transactions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about and Prepare() might still
be acquiring row locks.

I ran the new raft_consensus-itest that includes unique/duplicate
key workloads and exactly once semantics in dist-test.

Results of slow mode, 1 stress thread, 1000 loop runs of
raft_consensus-itest on dist-test.

TSAN-prev (16/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480814757.31031
TSAN-this (10/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480812541.27015

I inspected the TSAN failures and they were mostly ~DnsResolver()
races on test code itself. The tests themselves passed for the
most part. Some ended up timing out with archives that are too
big to download. This also happens in the previous patch.

ASAN-prev (02/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480817418.1140
ASAN-this (03/1000 failures): http://dist-test.cloudera.org//job?job_id=david.alves.1480811163.22558

I inspected the ASAN failures. Two of them are test only flakes.
One of them is worrying as the consensus queue remains full,
suggesting a deadlock, but this also happening on the previous
patch. Example:

https://kudu-test-results.s3.amazonaws.com/david.alves.1480817418.1140.36139843995986a54aaf9f533a00111f384e85a6.145.0-artifacts.zip?Signature=PjjGYVvmWns4t0M7j6%2FAZQYVj34%3D&Expires=1480904781&AWSAccessKeyId=AKIAJ2NR2VXMAHTVLMRA

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Reviewed-on: http://gerrit.cloudera.org:8080/5294
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
5 files changed, 60 insertions(+), 40 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about.

I ran the new raft_consensus-itest that includes unique/duplicate
key workloads and exaclty once semantics in dist-test with asan,
slow mode and 1 stress thread. 3/1000 tests failed. This is inline
with the baseline from the patch that changed the itest.

Results:
http://dist-test.cloudera.org//job?job_id=david.alves.1480768100.17891

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/14
--
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................

KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init() which we know is called by the thread
updating consensus, for non-leader transctions. This should be ok as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we now must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this, this patch adds a latch to make sure that, if replication
failed, the prepare phase has completed before returning from
ReplicationFinished().

Finally this patch adds a couple of statements to increase the
driver's refcount so that it survives until the latch may be called.

Ran this in dist-test, slow mode, asan, 1000 times. No failures.

Results:
exactly_once_writes-itest: http://dist-test.cloudera.org//job?job_id=david.alves.1480659005.9899
raft_consensus-itest     : http://dist-test.cloudera.org//job?job_id=david.alves.1480659414.10085

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 58 insertions(+), 26 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5294/12/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:

Line 430:     prepare_latch_.Wait();
thinking about this some more, I'm afraid of the following deadlock scenario:

- prepare queue has a bunch of operations which all touch the same row key, thus operation N cannot finish preparing until operation N-1 commits (due to the lock dependency)
- we're a replica, and we've prepared operation N-1
- operation N is sitting on the 'prepare' queue, blocked waiting for N-1 to commit
- a leader just occurred, and the leader is trying to abort operation N because it's being replaced

Now, I think we'd deadlock due to this new code path: the replica is waiting on the prepare_latch_ for N, but N can't prepare, because it's blocked on N-1's lock. but N-1 can't commit, because it needs the consensus thread to make progress before it can do so.

Does this make sense? Let me see if I can come up with a test for it against your patch to "prove it" as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................

KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread

To do this this patch adds a way to release only the mvcc txn,
since its the only thing we care about.

Results:
http://dist-test.cloudera.org//job?job_id=david.alves.1480768100.17891

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/15
--
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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-798 (part 3) Replica transactions must start/abort on the consensus update thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-798 (part 3) Replica transactions must start/abort on the consensus update thread
......................................................................


Patch Set 21:

Safe time advancement (https://gerrit.cloudera.org/#/c/5240/) would fail without this yeah.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@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

[kudu-CR] KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread
......................................................................

KUDU-798 (part 3) Make replica transactions start/abort on the consensus thread

In order for a consensus replica to safely move "safe" time, it needs
to know that all transactions that come before it have at least
started. One way to do this is to call transaction_->Start() on
transaction_driver_->Init() which we know is called by the thread
updating consensus for non-leader transctions. This should be ok, as
the leader has already correctly serialized the transactions.

However, if transactions are now started on Init() then, in the case
they abort (i.e. when transaction_driver_->ReplicationFinished() is
called with a non-ok status, again done by the thread updating
consensus), we now must make sure that the transaction is actually
removed from mvcc before that method returns. Otherwise consensus
might call Start() on another transaction with the same timestamp
before the failed transaction is removed from mvcc.

To do this, this patch adds a latch to make sure that, if replication
failed, the prepare phase has completed before returning from
ReplicationFinished().

Finally this patch adds a couple of statements to increase the
driver's refcount so that it survives until the latch may be called.

Ran this in dist-test, slow mode, asan, 1000 times. No failures.

Results:
exactly_once_writes-itest: http://dist-test.cloudera.org//job?job_id=david.alves.1480659005.9899
raft_consensus-itest     : http://dist-test.cloudera.org//job?job_id=david.alves.1480659414.10085

Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
---
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
2 files changed, 58 insertions(+), 26 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/5294/8
-- 
To view, visit http://gerrit.cloudera.org:8080/5294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie360e597eea86551c453717d7a1a000848027f4c
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>