You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2017/07/15 00:10:07 UTC
[kudu-CR] WIP disk failure: forced shutdown of MvccManager
Andrew Wong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7439
Change subject: WIP disk failure: forced shutdown of MvccManager
......................................................................
WIP disk failure: forced shutdown of MvccManager
When a disk fails, tablets with data on the failed disk must shut down.
Normally, when a replica is shutdown, it must wait for all transactions
to finish. However, transactions that fail due to disk failure will not
complete.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
cannot end a transaction that is in the APPLYING state (must be
RESERVED), Cancels can end any transaction. This is only "safe" because
the tablet to whom the MvccManager belongs is expected to shutdown.
WIP because there is no testing and I'm not sure this is the best design
for what I'm trying to accomplish.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica_mm_ops.cc
M src/kudu/tablet/transactions/alter_schema_transaction.h
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/transaction_tracker-test.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/tablet/transactions/transaction_tracker.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/ts_tablet_manager.cc
19 files changed, 187 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/1
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#27).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 655 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/27
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 27
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
> Patch Set 15:
>
> (1 comment)
The behavior I saw before was that when I stopped MVCC on a leader and continued to write to that leader, something would successfully complete and I would see a mismatch in attempt numbers, resulting in a check failure.
I think this is being caused by the following race with three nodes A*, B, and C:
1. A* gets a client request for rpc0
2. rpc0 gets replicated and the nodes begin applying with attempt 0
3. A* fails. The client will try different nodes, but if A* remains the leader, it will eventually cycle back to A* and try again, so it replicates again.
4. B and C begin to apply rpc0 attempt 1, registering it with the result tracker and preempting the follower transaction rpc0 attempt 0
5. B and C finish Applying rpc0 attempt 0 and fail because attempt 0 is no longer the driver
If I put a sleep in ResultTracker::RecordCompletionAndRespond(), the above scenario is triggered consistently. We do this preemption to ensure that we don't respond to a leader transaction that may no longer be valid, but I don't see where we might be aborting this follower transaction.
Currently dong more testing around this.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:02:46 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#32).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 646 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/32
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 32
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#20).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by closing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is closed, new transactions will
abort immediately before even reaching the Prepare phase.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
closed immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is closed and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- tests in ts_tablet_manager-itest that ensure stopped leaders don't
complete writes and stopped followers do.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
12 files changed, 425 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/20
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); that stopped tablets don't prevent
fault-tolerant scans; and that stopping the only tablet does prevent
scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
Reviewed-on: http://gerrit.cloudera.org:8080/7439
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 733 insertions(+), 135 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 40
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7439/14/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/14/src/kudu/integration-tests/ts_tablet_manager-itest.cc@40
PS14, Line 40: #include "kudu/gutil/strings/substitute.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.h@275
PS12, Line 275: / Once called, new transactions
> we now have std::atomic
Done
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.cc@98
PS12, Line 98: if (PREDICT_FALSE(shutdown_pending_.load())) {
> use PREDICT_FALSE
Done
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.cc@99
PS12, Line 99: LOG(WAR
> use LOG_EVERY_N or even just LOG, this is an important message that is not
Done
http://gerrit.cloudera.org:8080/#/c/7439/14/src/kudu/tablet/transactions/transaction_driver.h
File src/kudu/tablet/transactions/transaction_driver.h:
http://gerrit.cloudera.org:8080/#/c/7439/14/src/kudu/tablet/transactions/transaction_driver.h@237
PS14, Line 237: Status Init(gscoped_ptr<Transaction> transaction,
> warning: function 'kudu::tablet::TransactionDriver::Init' has a definition
Done
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/transactions/transaction_driver.cc@68
PS12, Line 68: using std::string;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 27 Sep 2017 23:07:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 27
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 21:
(26 comments)
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@77
PS20, Line 77: using itest::SimpleIntKeyKuduSchema;
> warning: using decl 'InternalMiniCluster' is unused [misc-unused-using-decl
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@130
PS20, Line 130: ASSERT_OK(cluster->Start());
> this should probably be less specific to implementation details
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@179
PS20, Line 179: FLAGS_enable_leader_failure_detection = false;
> I'm not sure this test suite is the best place for these tests, since they
I moved to stop_tablet-itest.cc
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@183
PS20, Line 183: FLAGS_allow_unsafe_replication_factor = true;
> here you're stopping a random server, not necessarily the one that the scan
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@187
PS20, Line 187:
> are the readers using fault-tolerant scans by default here?
Yeah they are (see test_workload.cc:219)
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@197
PS20, Line 197:
> Somewhere we have a utility function like AssertNoCrashes(). Otherwise mayb
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@205
PS20, Line 205: ValueDeleter deleter(&ts_map);
> in these tests it might be beneficial to configure the MM such that it cons
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@235
PS20, Line 235:
> is it possible in this test to re-start the tserver that has the stopped ta
I think it'd be a bit tricky in this patch since this makes use of IMC instead of EMC, unless there's a ClusterVerifier equivalent for IMCs that I'm not aware of. Good point though, in EMC tests I've been verifying with ClusterVerifier.
Trickier still, this actually makes use of the fact that we're using IMC by stopping the tablet directly by reaching into the server, so I'd prefer to keep this test IMC.
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@238
PS20, Line 238: // The MarkDirty() callback is on an async thread so it might take the
> can you combine this test with the above one, using a utility function that
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@498
PS20, Line 498: waiting_thread.join();
: ASSERT_OK(s);
> don't you want to join before assert here?
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@598
PS20, Line 598: waiting_thread.join();
: ASSERT_STR_CONTAINS(s.ToString(), "closed");
: ASSERT_TRUE(s.IsAborted());
:
: // New waiters should abort immediately.
: s = mgr.WaitForApplyingTransactionsToCommit();
:
> if you reordered the join and the ASSERT then you could avoid the complexit
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@253
PS20, Line 253: Status::
> specify the type of status
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@254
PS20, Line 254: Status WaitForApplyingTransactionsToCommit() const WARN_UNUSED_RESULT;
> worth adding a WARN_UNUSED_RESULT here
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@275
PS20, Line 275: not
> not?
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@280
PS20, Line 280: e bool i
> rename to is_closed since it's inlinable short method
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@160
PS20, Line 160: //
> mind adding a WARN_UNUSED_RESULT to these functions that you're changing fr
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405
PS20, Line 405:
> nit: "should" is kinda fuzzy. Do you mean "will"? Nicer for callers to thin
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405
PS20, Line 405:
: // Return handle to the metric entity of this tablet.
: const scoped
> Would be good to move this declaration up next to 'Shutdown()' and clarify
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@465
PS20, Line 465: // into an internal UPDATE operation and performs it.
> add doc for what bad status means. Same above with the Apply calls.
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@537
PS20, Line 537: // The 'old_ms' pointer will be set to the current MemRowSet set before the replacement.
: // If the MemRowSet is not empty it will be added to the 'compaction' input
: // and the MemRowSet compaction
> can't you just use the corresponding method in mvcc?
You're right, although sometimes it's convenient to condition on a the status, and sometimes it's convenient to condition a bool. E.g. if we need to return an error in multiple places if the tablet has been Stopped(), this is more convenient than repeating:
if (tablet->Stopped()) {
return Status::Aborted("aborted message");
}
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@806
PS20, Line 806: // In order for failures here to be safe, the tablet must have been
> I think it's worth a comment here explaining why we expect it to already ha
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@827
PS20, Line 827: p();
> nit: how about "Error while checking roe presence"
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@839
PS20, Line 839: w_ops_bas
> PREDICT_FALSE?
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@900
PS20, Line 900: ol present = false;
> can't you use CheckNotStopped here
Done
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet_replica.cc@263
PS20, Line 263:
> is it ok to access tablet_ outside of state_change_lock_? can't quite recal
From the comment around state_change_lock_, it seems it's only guarding long-running lifecycle operations, which this isn't. lock_ seems necessary though, since it accesses tablet_.
I think it was safe before just because we had the lock above serializing and deduplicating Stop() calls, but I'll add it up there.
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tserver/tablet_service.cc@1731
PS20, Line 1731: if (!s.ok()) {
: tmp_error_code = TabletServerErrorPB::INVALID_SNAPSHOT;
> this sentence seems incomplete, so what do we do?
Done, also moved this a bit lower and conditioned the error code on Stopped() instead.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:38:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] disk failure: release failed txs from tracker
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#6).
Change subject: disk failure: release failed txs from tracker
......................................................................
disk failure: release failed txs from tracker
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way to release transactions from the TransactionTracker
without crashing Kudu, regardless of state.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
ensure the transaction is not in the APPLYING state when destructed, a
Canceled transaction has no such constraint. This is only "safe" because
the TransactionTracker's tablet is expected to shut down and no longer
be used.
Additionally, a new flag is added to Tablet to indicate that it resides
on a failed disk. If set, the tablet's transactions will end early.
Currently, this codepath will not have a visible effect on Kudu as disk
failures are still fatal, but testing is still done to test when this
flag is manually set without crashes.
Testing is done in a couple ways:
- A test is added in mvcc-test to Cancel an Applying ScopedTransaction
and ensure that there are no errors when it leaves scope.
- A test is added to tablet_replica-test to register a WriteTransaction,
set the tablet's "in-failed-dir" flag, and being Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/alter_schema_transaction.h
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/transaction_tracker-test.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 133 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/6
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#21).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by closing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is closed, new transactions will
abort immediately before even reaching the Prepare phase.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
closed immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is closed and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders don't
complete writes and stopped followers do, and stopped tablets don't
prevent fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
13 files changed, 505 insertions(+), 75 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/21
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 12:
Right, that's definitely a valid concern: on-disk isn't the only place we can "persist" the data. Right now, the only time we'd ever get in this "aborting transactions without them committing" state is when we're shutting down the replica, so we should be safe in this patch.
That said, a fact of life is that we'll need to synchronize the tablet's new state and the replica. Yes, there is a slight change in this patch at TabletReplica to do this at shutdown, but when it comes time to do this at disk-failure-time, this will need to be taken care of.
One way to do that would be to change the replica's state synchronously (e.g. to SHUTDOWN_PENDING, HANDLING_FAILURE, or somesuch), and then synchronously shut down the tablet's MVCC manager. In doing so, the replica _should_ stop responding to scan requests and copy requests. Work for another patch though. This one has roughly what I described: https://gerrit.cloudera.org/#/c/7442/
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#28).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 646 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/28
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 28
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
This is one of the most tricky changes (if not the most tricky) in the whole patch series, IMO. I would like to see this new failure mode sprinkled over other integration tests that actually stress consensus and transactions so that we're confident things are have not been broken. exactly_once_writes-itest, raft_consensus-itest, linked_list-test come to mind but there might be other/better candidates
It might be impossible to test this independently. If that is the case I'd be happy to have a test-only patch in the sequence (as close as possible to this one) where we exercise the complete failure mode and that includes several different dist-test runs.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 27 Sep 2017 23:50:13 +0000
Gerrit-HasComments: No
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 20:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@537
PS20, Line 537: // Returns an error if the tablet has been stopped, e.g. because the tablet
: // needs to be closed due to an error.
: Status CheckNotStopped() const;
can't you just use the corresponding method in mvcc?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@827
PS20, Line 827: Failed to check presence of rows
nit: how about "Error while checking roe presence"
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@839
PS20, Line 839: Stopped()
PREDICT_FALSE?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@900
PS20, Line 900: return Status::Aborted("Transaction failed because tablet was stopped");
can't you use CheckNotStopped here
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tserver/tablet_service.cc@1731
PS20, Line 1731: // If we get a Status::Aborted() from HandleScanAtSnapshot(), it likely
: // means the tablet had to shut down, e.g. due to a disk error.
this sentence seems incomplete, so what do we do?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 27 Oct 2017 20:06:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#9).
Change subject: Allow tablet shutdown without completing txs
......................................................................
Allow tablet shutdown without completing txs
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way ensure the transactions are released without crashing
the server, regardless of state.
This patch adds the ability to mark a tablet's MvccManager as shutting
down, after which Applies will exit early, responding with an Aborted
status.
Testing is done by adding the following:
- a test in mvcc-test to shut down the manager and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in tablet_replica-test to register a WriteTransaction,
shut down the tablet's MvccManager, and begin Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
9 files changed, 136 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/9
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: release failed txs from tracker
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#8).
Change subject: disk failure: release failed txs from tracker
......................................................................
disk failure: release failed txs from tracker
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way to release transactions from the TransactionTracker
without crashing Kudu, regardless of state.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
ensure the transaction is not in the APPLYING state when destructed, a
Canceled transaction has no such constraint. This is only "safe" because
the TransactionTracker's tablet is expected to shut down and no longer
be used.
Additionally, a new flag is added to Tablet to indicate that it resides
on a failed disk. If set, the tablet's transactions will end early.
Currently, this codepath will not have a visible effect on Kudu as disk
failures are still fatal, but it is tested for.
Testing is done in a couple ways:
- A test is added in mvcc-test to Cancel an Applying ScopedTransaction
and ensure that there are no errors when it leaves scope.
- A test is added to tablet_replica-test to register a WriteTransaction,
set the tablet's "in-failed-dir" flag, and begin Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/alter_schema_transaction.h
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/transaction_tracker-test.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 133 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/8
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#18).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by closing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is closed, new transactions will
abort immediately before even reaching the Prepare phase.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
closed immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is closed and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- tests in ts_tablet_manager-itest that ensure stopped leaders don't
complete writes and stopped followers do.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
13 files changed, 425 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/18
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 18
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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 disk failure: forced shutdown of replicas
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP disk failure: forced shutdown of replicas
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/4//COMMIT_MSG
Commit Message:
PS4, Line 15: cannot end a transaction that is in the APPLYING state (must be
: RESERVED), Cancels can end any transaction. This is only "safe" because
: t
currently I think we directly enter the 'APPLYING' state when we submit a transaction to the 'apply_pool', right? But in fact it could be either be (a) in the queue waiting to start actually applying, or (b) actually in the process of applying.
In the first case (waiting in the queue) this seems reasonable and safe to do. In fact we should probably be doing this in _all_ tablet "shutdown" scenarios (eg DeleteTablet is a common case). If a transaction is still waiting to start applying, then simply aborting it and having it be a no-op when it exits the queue into the 'Apply' code path is safe (I don't think we have any clean way to remove it from the queue while waiting).
In the second case (actually inside WriteTransaction::Apply) I don't think we can safely go in and do anything to the transaction object, and we truly do need to wait for it to finish, right? We could of course set some flag on the tablet itself indicating that it is being shut down, and peroidically check that flag while processing writes, and then Abort the transaction if we see it get set mid-write, but that's best-effort and essentially an optimization, right?
As for "Abort" vs "Cancel", I'm trying to figure out what the distinction is here. I'm not necessarily against adding a new API, but we should be clear what the distinction is and whether it actually needs to behave any differently. From the transaction's view point, either is the same, right? In both cases you just want it to stop in its tracks as soon as possible and not write any COMMIT message to the log.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#14).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by shutting down its MVCC
manager. Once this happens, Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is shut down, new transactions will
abort immediately.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
stopped immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is shut down and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- tests in ts_tablet_manager-itest that ensure stopped leaders don't
complete writes and stopped followers do.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
14 files changed, 422 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/14
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 14
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
(3 comments)
> Patch Set 15:
>
> > Patch Set 15:
> >
> > (1 comment)
>
> The behavior I saw before was that when I stopped MVCC on a leader and continued to write to that leader, something would successfully complete and I would see a mismatch in attempt numbers, resulting in a check failure.
>
> I think this is being caused by the following race with three nodes A*, B, and C:
> 1. A* gets a client request for rpc0
> 2. rpc0 gets replicated and the nodes begin applying with attempt 0
> 3. A* fails. The client will try different nodes, but if A* remains the leader, it will eventually cycle back to A* and try again, so it replicates again.
> 4. B and C begin to apply rpc0 attempt 1, registering it with the result tracker and preempting the follower transaction rpc0 attempt 0
> 5. B and C finish Applying rpc0 attempt 0 and fail because attempt 0 is no longer the driver
>
> If I put a sleep in ResultTracker::RecordCompletionAndRespond(), the above scenario is triggered consistently. We do this preemption to ensure that we don't respond to a leader transaction that may no longer be valid, but I don't see where we might be aborting this follower transaction.
>
> Currently dong more testing around this.
We just had another in-person chat that I thought I'd summarize:
This race is dependent on the fact that we're aborting _after_ replicating to followers. This may not be possible on the master branch.
So A* is aborting during the leader's Apply phase, causing the client to retry and cycle through replicas searching for the leader (since before we would abort if the replica were not a leader). Eventually it would reach the same replica and successfully replicate the same RPC _again_.
The followers OTOH are on their merry way Applying this successfully-replicated attempt0. When the RPC gets replicated the second time, followers preempt the current attempt and eventually crash because of it.
Getting back to the question of where to abort, I think Init() or Prepare() would do the trick, I would think the earlier the better. Either _should_ prevent this, but I don't see the point in even submitting to the Prepare pool.
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/mvcc.h@277
PS15, Line 277: SetShuttingDown
> since there is no "action" this entity is doing maybe "closed" with be a be
Done
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.h@405
PS15, Line 405: // Stops the tablet. Once called, currently-Applying operations should
: // terminate early, and further transactions should be aborted.
: void Stop();
:
: bool Stopped() const {
: return mvcc_.IsShuttingDown();
: }
> I don't think we need this in tablet, since it's a plain proxy to mvcc whic
I added this in per Todd's request. You're right that we don't _need_ this in tablet, but it's a cleaner API than reaching into MVCC directly.
I suppose the argument for it would be more compelling if there were more things that needed to be stopped on Stop().
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.cc@438
PS15, Line 438: gscoped_ptr<ScopedTransaction> mvcc_tx;
: DCHECK(tx_state->has_timestamp());
: mvcc_tx.reset(new ScopedTransaction(&mvcc_, tx_state->timestamp()));
: tx_state->SetMvccTx(std::move(mvcc_tx));
> I though we had discussed having transactions fail on the prepare phase if
My thinking was that Prepare could only be run if it were first Inited. There may be some scenario between Init() and Prepare() the disk fails, but in that case, once the transaction is replicated, this should fail in the Apply phase. I.e. nothing should really change in the Prepare phase (yet) because the Prepare phase only touches the WAL dir.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 03 Oct 2017 21:08:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#29).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 646 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/29
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 29
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#31).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 646 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/31
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 31
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 34:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@137
PS33, Line 137: // Ensure that stopping a tablet will not prevent scans from completing.
> We don't need to be guaranteed that the specific replica that has been stop
Why use multiple replicas if we want to test that?
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@200
PS33, Line 200: a new leader will not be elected
> I've looped this a couple thousand times with some stress in TSAN and only
Well written tests don't fail on stressed TSAN. Do those "normal" errors include this test?
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h@85
PS32, Line 85: Status Flush() const;
> Hrm, sorry for the distraction, I was confused about why it wasn't, since i
I'd argue we shouldn't do this, since it's basically a c++ bug that this even works. We're "transitively" mutating something, even if it's not the first-hop member of this method, which happens to be a shared_ptr.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 34
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 08:15:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#25).
Change subject: introduce closed mvcc and stopped tablets
......................................................................
introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
16 files changed, 636 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/25
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 25
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#19).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by closing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is closed, new transactions will
abort immediately before even reaching the Prepare phase.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
closed immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is closed and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- tests in ts_tablet_manager-itest that ensure stopped leaders don't
complete writes and stopped followers do.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
12 files changed, 425 insertions(+), 116 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/19
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 19
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] disk failure: release failed txs from tracker
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: release failed txs from tracker
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7439/6/src/kudu/tablet/transactions/transaction.h
File src/kudu/tablet/transactions/transaction.h:
Line 20: #include <string>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7439/6/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
Line 45: using std::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/7439/6/src/kudu/tablet/transactions/transaction_tracker-test.cc
File src/kudu/tablet/transactions/transaction_tracker-test.cc:
Line 24: #include "kudu/tablet/transactions/transaction_driver.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#24).
Change subject: introduce closed mvcc and stopped tablets
......................................................................
introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
16 files changed, 638 insertions(+), 118 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/24
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 24
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 12:
Posting a transcript of a side chat, for posterity:
...
[1:12 PM] David Alves: no, no other transaction with a timestamp t1 > t" should be _made visible_ for the tablet
[1:12 PM] David Alves: (not the tserver)
[1:12 PM] Andrew Wong: ah ok
[1:12 PM] Andrew Wong: good
[1:12 PM] David Alves: that's what I meant that I don't care too much about actually stopping txns from applying
[1:13 PM] David Alves: but I do care that no new scan at a timestmap > t can start (or continue)
[1:13 PM] Andrew Wong: but also i was wondering whether there were any preclusions if we want to support multi-tablet txns
[1:13 PM] Andrew Wong: as far as things go _now_, i think we can enforce that by changing the replica state
[1:13 PM] David Alves: not sure that is true
[1:14 PM] Andrew Wong: oh?
[1:14 PM] Andrew Wong: hrmm i thought we'd only scan or copy if the replica were RUNNING
[1:14 PM] Andrew Wong: let me check that
[1:14 PM] Andrew Wong: or do you mean if we go mid-scan / mid-copy
[1:14 PM] David Alves: I think there's opportunity for a scan to get a replica state of RUNNING, then be assigned a timestamp that is > t
[1:14 PM] David Alves: yeah
[1:15 PM] David Alves: so I was suggesting that, instead of transpiring the status of the apply to the transcation driver, you could instead change the state of the mvcc manager internally
[1:16 PM] David Alves: and pass along a shutdown timestamp
[1:16 PM] David Alves: and then, when creating a snapshot on mvcc you would check whether the snapshot timestmap is lower than the shutdown timestamp
[1:17 PM] David Alves: this also has the advantage of simplifying your patch, no changes to the transaction driver are required
[1:21 PM] Andrew Wong: why bother snapshotting at all if the tablet is going to be FAILED/shut down anyway?
[1:22 PM] Andrew Wong: instead of aborting or something
[1:25 PM] Andrew Wong: brb grabbing some lunch
[1:25 PM] David Alves: was doing the same
[1:25 PM] David Alves: yeah, that's kind of what I meant
[1:27 PM] David Alves: oh, wait. yeah, I guess maybe we don't need the timestmap at all
[1:28 PM] David Alves: but we do need the mvcc manager to return a non-ok status for trasactions that were wating on timestamps equal to or greater than t to be "consistent"
[1:29 PM] David Alves: so, in summary:
[1:30 PM] David Alves: - the tablet can (and must) shutdown the mvcc manager internally. when this happens it should break all pending scans waiting for timestamp >= t to be consistent
[1:31 PM] David Alves: eventually this will propagate to the tablet replica (through the asynch callback triggered by the error manager) and scans won't even have the chance to start
[1:31 PM] David Alves: sounds reasonable?
[1:33 PM] Andrew Wong: Mostly, but why not abort all scans?
[1:39 PM] David Alves: what do you mean?
[1:41 PM] Andrew Wong:
| - the tablet can (and must) shutdown the mvcc manager internally. when this happens it should break all pending scans waiting for timestamp >= t to be consistent
| Shutting down mvcc should about all scans to the tablet, right?
[1:41 PM] Andrew Wong: Should abort*
[1:42 PM] David Alves: only "consistent" scans actually wait for timestamps
[1:42 PM] David Alves: the other ones don't. they take a snapshot of mvcc and move forth
[1:42 PM] David Alves: don't see how you can abort those
[1:42 PM] David Alves: since they don't check back with mvcc
[1:43 PM] David Alves: they will once they see replica in non-RUNNING state
[1:43 PM] David Alves: but I don't think we should concern ourselves with these scans too much
[1:46 PM] Andrew Wong: Ah I see
[1:47 PM] Andrew Wong: Yeah sounds reasonable
[1:48 PM] Andrew Wong: I'll also consider whether setting the replica state synchonously makes sense. I think it does, hopefully there won't be races
[1:54 PM] David Alves: that I'm not so sure of
[1:55 PM] David Alves: particularly as there's work to do
[1:55 PM] David Alves: in that case
[1:55 PM] David Alves: which opens the possiblity of deadlocks, etc
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] WIP disk failure: forced shutdown of replicas
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#5).
Change subject: WIP disk failure: forced shutdown of replicas
......................................................................
WIP disk failure: forced shutdown of replicas
When a disk fails, replicas with data on the failed disk must shut
down. Normally, when a replica is shutdown, it must wait for all
transactions to finish. However, transactions that fail due to disk
failure will not complete.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
cannot end a transaction that is in the APPLYING state (must be
RESERVED), Cancels can end any transaction. This is only "safe" because
the tablet to whom the MvccManager belongs is expected to shutdown.
In addition to this, a new tablet state is added: FAILED_AND_SHUTDOWN.
Like QUIESCING and SHUTDOWN, TabletReplica::Shutdown() can wait on
FAILED_AND_SHUTDOWN. This is needed to shut down failed tablets that
need to be replicated: calling Shutdown() cannot leave the replica in
the FAILED state, and the SHUTDOWN state cannot itself indicate the
need for a tablet copy.
WIP because there is no testing and I'm not sure this is the best design
for what I'm trying to accomplish.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/alter_schema_transaction.h
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/transaction_tracker-test.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/tablet/transactions/transaction_tracker.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
14 files changed, 107 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/5
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 38: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 38
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:08:02 +0000
Gerrit-HasComments: No
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#13).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit. This constraint was put in place to guarantee
on-disk correctness, but is sometimes too strict. E.g. if the tablet is
shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by shutting down its MVCC
manager. Once this happens, Applies will return and not move on to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
this behavior of having some in-flight transactions Commit and others
not is consistent with the server crashing in between the Commits of two
transactions.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
stopped immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is shut down and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a test in tablet_replica-test to register a WriteTransaction,
shut down the tablet's MvccManager, and begin Applying. The
transaction exits early and releases itself from MVCC.
- integration tests in ts_tablet_manager-itest that ensure workloads
complete despite having failed tablets.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
15 files changed, 401 insertions(+), 106 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/13
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 13
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#35).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 676 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/35
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 35
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 11:
(12 comments)
Not yet ready for review.
http://gerrit.cloudera.org:8080/#/c/7439/11//COMMIT_MSG
Commit Message:
Line 21: Testing is done by adding the following:
> forgot to ask: does TestDeleteTableWithConcurrentWrites in delete_table-ite
Ah failed occasionally on a previous iteration, but since updating it, I haven't looped it. Will do that now.
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS11, Line 270: transactions must not durably complete operations
> is this precisely true? or is it that transactions are _allowed_ to abort?
They _may_ not durably complete operations.
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
PS11, Line 764: message
> use ToString so we retain the original code?
Done
Line 765: LOG(ERROR) << s.ToString();
> add context of the tablet id, maybe the opid which should be available?
Done
PS11, Line 800: exitted
> typo? exited
Done
PS11, Line 799: if (PREDICT_FALSE(mvcc_.IsShuttingDown())) {
: return Status::Aborted("Transaction exitted early because tablet is shutting down");
: }
> mind making a short function like Tablet::CheckNotShuttingDown with this co
Done
Line 852: LOG(ERROR) << "Failed to check if row is present; ending transaction early";
> we should be able to CHECK(IsShuttingDown()) or something here, right?
Done
Line 853: CHECK(s.IsDiskFailure());
> I think this is a bit too restrictive. For example if there is some local h
Done
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
Line 347: if (!state()->tablet_replica()->tablet()->mvcc_manager()->IsShuttingDown()) {
> I think reaching into MvccManager is a bit strange here. Even though there
The difficulty with using the TabletReplica enum is that it's protected by a lock. We need an immediately-available state to ensure that transactions are not committed, since we've lost the durability guarantee.
My justification for this sort of desynchronization of tablet_replica/tablet is that the tablet--the physical guts storing all of the tablet meta/data--is what's failing when we enter this state; the replica merely follows suit asynchronously.
Line 352: }
> can you add FALLTHROUGH_INTENDED here?
Done
PS11, Line 356: VLOG_WITH_PREFIX(1) << "Transaction " << ToString() << " failed prior to "
: "replication success: " << s.ToString();
> this is no longer accurate in all cases
Done
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
PS11, Line 640: if (replica->tablet()) {
: // If we're deleting the tablet, there's no point in ensuring that applies
: // complete. Currently-running Applies will henceforth return early.
: replica->tablet()->mvcc_manager()->SetShuttingDown();
: }
:
> why not put this in TabletReplica::Shutdown() implementation itself so as t
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 35:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@137
PS33, Line 137: // Ensure that stopping a tablet will not prevent scans from completing.
> Why use multiple replicas if we want to test that?
1/3 replicas stopping shouldn't affect fault-tolerant scans but 1/1 will (the TestWorkload wouldn't be able to open a scanner and the test would crash). Added a comment.
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@200
PS33, Line 200:
> Well written tests don't fail on stressed TSAN. Do those "normal" errors in
Oh hrm, it seems I had the wrong idea of stressed tsan tests and that these were due to the bugs todd fixed. Regardless, there were no errors due to a failure in this test.
With Todd's patch, this gets through 1000/1000 with --cpu_stress_threads=8.
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h@85
PS32, Line 85: Status Flush();
> I'd argue we shouldn't do this, since it's basically a c++ bug that this ev
Makes sense, done
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc@952
PS33, Line 952: case RowOperationsPB::UPDATE:
: case RowOperationsPB::DELETE:
: ignore_result(MutateRowUnlocked(tx_state, row_op, stats));
: return Status::OK();
:
: default:
: LOG_WITH_PREFIX(FATAL) << RowOperationsPB::Type_Name(row_op->decoded_op.type);
: }
: return Status::OK();
: }
:
: void Tablet::ModifyRowSetTree(const RowSetTree& old_tree,
:
> Would you mind splitting these changes into their own patch? They already h
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 35
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 09:24:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 23:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/7439/23//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7439/23//COMMIT_MSG@49
PS23, Line 49: a new test stop_tablet-itest is added to ensure stopped leaders don't
: complete writes and stopped followers do
> having trouble parsing this. why do followers complete writes?
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@57
PS23, Line 57: using std::thread;
> warning: using decl 'thread' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@82
PS23, Line 82: *ret = *ret %
> nit: %=
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@90
PS23, Line 90: T $0 P $1
> nit: usually we do P ... T ... (peer before tablet id)
Hrm, looked around and it seems we mostly use "T xxx P xxx"? That's how it is in master/sys_catalog.h, and upon `grep`ing for " P "
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@102
PS23, Line 102: NO_PENDING_FATALS();
> I don't think this has any effect - it would just return if there is a pend
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@129
PS23, Line 129: // or -1 if it can't be found.
> sort of odd to return -1 instead of bad status here. Up above GetTserverNum
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@181
PS23, Line 181: If this returns an error, the Tablet must
: // have been stopped.
> Similar to some comments on earlier revs, I find this doc hard to interpret
I came to the realization that this might not be an appropriate place for this sort of comment. Errors can be passed up and down the stack and it shouldn't be the burden of one of the intermediate methods to bear the checking.
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@190
PS23, Line 190: // have been stopped.
> same as above
Same
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@230
PS23, Line 230: Status RewindSchemaForBootstrap(const Schema& schema,
> warning: function 'kudu::tablet::Tablet::RewindSchemaForBootstrap' has a de
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@382
PS23, Line 382: Status DoMajorDeltaCompaction(const std::vector<ColumnId>& column_ids,
> warning: function 'kudu::tablet::Tablet::DoMajorDeltaCompaction' has a defi
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@399
PS23, Line 399: void RegisterMaintenanceOps(MaintenanceManager* maintenance_manager);
> warning: function 'kudu::tablet::Tablet::RegisterMaintenanceOps' has a defi
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@408
PS23, Line 408: // This method is not thread safe.
> nit: it seems this is just copying from above, but would be nice if this cl
Hrm, did this by pushing it under a lock since it's pretty cheap. The lock-holding semantics of the lock are a bit difficult to express.
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@465
PS23, Line 465: void set_state(State s) {
> maybe name set_state_unlocked
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@466
PS23, Line 466: state_ = s;
> can you add a DCHECK assert on state_lock_ here?
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@594
PS23, Line 594: // ones specified by 'expected_states'.
> it is surprising to me that passing an empty set here, then, has the effect
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@597
PS23, Line 597: Status CheckActiveState(const std::set<State>& expected_states = {}) const;
> given that we expect the set to be very small, I think a vector is probably
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@683
PS23, Line 683: potecting
> typo
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@299
PS23, Line 299: &&
> you mean ||?
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@879
PS23, Line 879: expected_states.empty()
> is this necessary?
I was trying to replace:
CHECK(state_ == kOpen || state_ == kBootstrapping);
with
{
std::lock_guard<simple_spinlock> l(state_lock_);
if (state_ == kStopped || state_ == kShutdown) {
return Status::Aborted();
}
CHECK(state_ == kOpen || state_ == kBootstrapping);
}
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@1028
PS23, Line 1028: shared_ptr<RowSet> input_rs) {
> warning: the parameter 'input_rs' is copied for each invocation but only us
Done
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@1120
PS23, Line 1120: kOpen, kBootstrapping
> how is this different than checking for {}? see other comment about maybe s
Ack
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tserver/tablet_service.cc@1761
PS23, Line 1761: LOG(WARNING) << "Error setting up scanner with request " << SecureShortDebugString(*req);
> this should probably include s.ToString()
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 23
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 15 Nov 2017 21:37:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] disk failure: release failed txs from tracker
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: disk failure: release failed txs from tracker
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/4//COMMIT_MSG
Commit Message:
PS4, Line 15: without crashing Kudu, regardless of state.
:
: T
> currently I think we directly enter the 'APPLYING' state when we submit a t
Realized I never clicked respond.
Right, if the transaction is waiting in the queue, there's not much we can do. When the transaction starts applying, however, we can check if the tablet has been failed and return early.
Yeah, a flag to check whether the tablet is on a failed disk (not shut down quite yet) should do the trick (I have essentially that in a separate patch). I think it's fair game to end the transaction mid-apply (e.g. in ApplyRowOperations), lest we fail some checks if the apply is the thing that's failing.
I agree that the distinction between "Abort" and "Cancel" is a bit (maybe too) subtle. The necessity comes from the fact that, for write transactions, the TransactionDrivers have ScopedTransaction objects that call MvccManager::AbortTransaction when they're deleted (e.g. when the driver is released).
The idea with "Cancel" is that we should be able to safely eject the transaction from the TransactionTracker (deleting the ScopedTransaction, and effectively "completing" the Apply, without ever officially committing the tx, and not have to worry about the constraints of the MvccManager that crash Kudu if aborting during an Apply.
Maybe a cleaner API would be to have some TransactionTracker::Abort(TransactionDriver) that would release the driver regardless of state, but that'd be more of a cosmetic change. The underlying behavior would be the same.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#10).
Change subject: Allow tablet shutdown without completing txs
......................................................................
Allow tablet shutdown without completing txs
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way ensure the transactions are released without crashing
the server, regardless of state.
This patch adds the ability to mark a tablet's MvccManager as shutting
down, after which Applies will exit early, responding with an Aborted
status.
Testing is done by adding the following:
- a test in mvcc-test to shut down the manager and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in tablet_replica-test to register a WriteTransaction,
shut down the tablet's MvccManager, and begin Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
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/tserver/ts_tablet_manager.cc
10 files changed, 141 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/10
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 24:
(3 comments)
Fixed tidy warnings. This is ready for review.
http://gerrit.cloudera.org:8080/#/c/7439/24/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/24/src/kudu/tablet/tablet.h@372
PS24, Line 372: // TODO: Handle MVCC to support MemRowSet and handle deltas in DeltaMemStore
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/7439/24/src/kudu/tablet/tablet.h@390
PS24, Line 390: void RegisterMaintenanceOps(MaintenanceManager* maint_manager);
> warning: function 'kudu::tablet::Tablet::RegisterMaintenanceOps' has a defi
Done
http://gerrit.cloudera.org:8080/#/c/7439/24/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/24/src/kudu/tablet/tablet.cc@179
PS24, Line 179: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 24
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 15 Nov 2017 22:11:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#26).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 658 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/26
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 26
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#23).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders don't
complete writes and stopped followers do, and stopped tablets don't
prevent fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
16 files changed, 631 insertions(+), 111 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/23
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 23
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 22:
> Patch Set 22:
>
> (1 comment)
Yeah, I think that makes sense, given we do something similar in tablet_replica. Will just have to be particularly careful around access to the lock and ABA issues, etc. Working on it.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 04 Nov 2017 01:31:58 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 37:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/36/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/36/src/kudu/integration-tests/stop_tablet-itest.cc@138
PS36, Line 138: *leader_num = i;
> It seems like this works but I'm worried that if someone changes the read t
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 37
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 21:57:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#11).
Change subject: Allow tablet shutdown without completing txs
......................................................................
Allow tablet shutdown without completing txs
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way ensure the transactions are released without crashing
the server, regardless of state.
This patch adds the ability to mark a tablet's MvccManager as shutting
down, after which Applies will exit early, responding with an Aborted
status.
Testing is done by adding the following:
- a test in mvcc-test to shut down the manager and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in tablet_replica-test to register a WriteTransaction,
shut down the tablet's MvccManager, and begin Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
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/tserver/ts_tablet_manager.cc
10 files changed, 137 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/11
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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 disk failure: forced shutdown of replicas
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#3).
Change subject: WIP disk failure: forced shutdown of replicas
......................................................................
WIP disk failure: forced shutdown of replicas
When a disk fails, replicas with data on the failed disk must shut
down. Normally, when a replica is shutdown, it must wait for all
transactions to finish. However, transactions that fail due to disk
failure will not complete.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
cannot end a transaction that is in the APPLYING state (must be
RESERVED), Cancels can end any transaction. This is only "safe" because
the tablet to whom the MvccManager belongs is expected to shutdown.
In addition to this, a new tablet state is added: FAILED_AND_SHUTDOWN.
Like QUIESCING and SHUTDOWN, TabletReplica::Shutdown() can wait on
FAILED_AND_SHUTDOWN. This is needed to shut down failed tablets that
need to be replicated: calling Shutdown() cannot leave the replica in
the FAILED state, and the SHUTDOWN state cannot itself indicate the
need for a tablet copy.
WIP because there is no testing and I'm not sure this is the best design
for what I'm trying to accomplish.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/alter_schema_transaction.h
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/tablet/transactions/transaction_tracker.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/ts_tablet_manager.cc
14 files changed, 100 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/3
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/11//COMMIT_MSG
Commit Message:
Line 21: Testing is done by adding the following:
forgot to ask: does TestDeleteTableWithConcurrentWrites in delete_table-itest also cover this code pretty well? I think it's worth looping that one.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 8:
(7 comments)
Also as per our slack discussion, rather than making use of tablet::FAILED, which in https://gerrit.cloudera.org/#/c/7440/ I'm making a "final" state, I went with your suggestion of shutting down the MvccManager. There's still a separation of replica and tablet, but I think this at least makes sense w.r.t. transactions.
Also note in on of my review comments that the MvccManager::SetShuttingDown() isn't really used here, but will be in the ErrorNotificationCb.
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
Line 404: void Cancel();
> here's an idea: how about rather than specifically cancelling each transact
Done
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/transactions/transaction.h
File src/kudu/tablet/transactions/transaction.h:
Line 20: #include <mutex>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
Line 481: CHECK(s.IsDiskFailure());
> per conversation on Slack, I think generalizing this to something like the
I updated HandleFailure to allow replicating/replicated failures to go through if the tablet is shutting down. The drive is able to get the necessary info (i.e. whether mvcc is shut down) through the TransactionState.
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:
Line 134: if (PREDICT_FALSE(tablet->IsInFailedDir())) {
> making ApplyRowOperations return a Status would allow centralizing this log
Done
Line 139: tablet->ApplyRowOperations(state());
> here if Tablet indicated that it has shut down then you can early-return wi
Done
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
Line 1076: tablet->SetInFailedDir();
> again maybe generalize to just 'MarkFailed' since the handling isn't partic
Since this codepath is, in this patch, still fatal, I've reverted these changes. The code here *would* be:
>tablet->mvcc_manager()->SetShuttingDown();
Line 1078: LOG(FATAL) << Substitute("Data directory $0 failed. Disk failure handling not implemented",
> now implemented?
Not quite, I moved other parts of failure handling (i.e. submission of Shutdown() call, permissiveness failed Flushes) to another patch to be "easier to review," and kept this fatal. Only updated here to show where the new code would be used.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] mvcc: allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 20:
It'd be better to have a itest that stresses this with real conditions within this patch along with a few results from dist test runs. maybe merge https://gerrit.cloudera.org/#/c/7031/ here?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 27 Oct 2017 20:08:12 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#38).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); that stopped tablets don't prevent
fault-tolerant scans; and that stopping the only tablet does prevent
scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 735 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/38
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 38
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/11//COMMIT_MSG
Commit Message:
Line 21: Testing is done by adding the following:
> forgot to ask: does TestDeleteTableWithConcurrentWrites in delete_table-ite
sorry for the email flood, but a couple more test suggestions:
- TestDeleteTableWithConcurrentWrites focuses on complete deletion of a table during a write workload, and expects the writes to start getting a NotFound error since the table is gone.
- We should add another which shuts down (eg by injecting a failure) just a single replica of a repl=3 table and ensures that the client does _not_ see any errors. ie it should see the "Aborted" errors and transparently fail over to another replica. It should then restart the server so that the tablet is available again and make sure that it can recover properly. Hopefully this test should be randomized enough to sometimes pick a leader, sometimes a follower.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 38:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/37/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/37/src/kudu/integration-tests/stop_tablet-itest.cc@193
PS37, Line 193: ASSERT_TRUE(s.IsTimedOut())
> nit: add << s.ToString() so this is diagnosable when / if it fails in the f
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 38
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:05:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 34:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@137
PS33, Line 137: // Ensure that stopping a tablet will not prevent scans from completing.
> I don't really understand what is being tested here. With 3 replicas, what
We don't need to be guaranteed that the specific replica that has been stopped is being scanned, since the property we're trying to enforce here is that a single stopped replica won't stop scans (and we can rely on slight RNG to ensure we have coverage for stopped vs not stopped).
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@200
PS33, Line 200: a new leader will not be elected
> This will likely be flaky because under load, IO blips can cause new leader
I've looped this a couple thousand times with some stress in TSAN and only seen "normal" stressed tsan errors
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h@85
PS32, Line 85: Status Flush() const;
> nit: it's pretty weird to mark this const, since it's technically mutating
Hrm, sorry for the distraction, I was confused about why it wasn't, since it's not mutating the RowSetMetadata. I came to the conclusion that only CommitUpdate() was mutating the in-memory state by mentally marking this as const, so I thought I'd actually mark it const.
Should've been in a different patch.
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@371
PS32, Line 371: col_ids
> nit: just for future reference, doing random renames in a big patch is a bi
This was to appease tidybot, which I suppose isn't all too important; sorry about that.
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@668
PS32, Line 668:
> nits:
Done
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.h@457
PS31, Line 457: switch (s) {
> it would be nice to assert on the state transitions in this function to enf
Done
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc@320
PS31, Line 320: CancelMaintenanceOps();
> Why do we need to make this call in Stop() ? Can't the ops just cancel them
Hrmm, I suppose they could. I put it here since the convention seemed to be that the "owner" of the op should control the lifecycle of op.
Also a single call like this makes use of the fact that ops are subclassed (vs if we had them check the tablet state, we'd put that checking code in each op's implementation).
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc@1398
PS31, Line 1398: // operation to finish in Unregister(), a different one can't get re-scheduled.
> If we don't want to hold the lock while unregistering the ops then we need
Done. This function is only ever called from a single thread, so copying is sufficient.
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.cc@1389
PS32, Line 1389: maintenance_ops_.swap(maintenance_ops);
: }
> is this sufficient protection here for this method? My worry is that, if yo
It is possible that the Stop() would cancel nothing, but UnregisterOps() and RegisterOps() are externally synchronized (in bootstrapping code and replica shutdown) in such a way that this would never be called at the same time as UnregisterOps(). Will add a DFAKE_MUTEX to enforce this.
And it's ok to cancel nothing here because a call to UnregisterOps() should be impending.
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc@874
PS33, Line 874: }
> warning: redundant boolean literal in conditional return statement [readabi
Done
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc@952
PS33, Line 952: }
: return s;
:
: case RowOperationsPB::UPDATE:
: case RowOperationsPB::DELETE:
: s = MutateRowUnlocked(tx_state, row_op, stats);
: if (s.IsNotFound()) {
: return Status::OK();
: }
: return s;
:
: default:
: LOG_WITH_
> these look like good changes... but are they at all related to the theme of
Hrm, somewhat, somewhat not. A part of this patch is about handling errors in the stopped state, where before we would not expect errors.
This is a case where before we would never get errors, but we can conceivably have errors here, and we should handle them appropriately, since this is on the Apply path.
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/transactions/transaction_driver.cc@518
PS31, Line 518: HandleFailure(s);
> Doesn't HandleFailure already do this? As long as we document that it takes
Done
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tserver/tablet_service.cc@1764
PS31, Line 1764: fail
> typo: fail
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 34
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 07:38:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 23:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/7439/23//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/7439/23//COMMIT_MSG@49
PS23, Line 49: a new test stop_tablet-itest is added to ensure stopped leaders don't
: complete writes and stopped followers do
having trouble parsing this. why do followers complete writes?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@82
PS23, Line 82: *ret = *ret %
nit: %=
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@90
PS23, Line 90: T $0 P $1
nit: usually we do P ... T ... (peer before tablet id)
Probably better to be consistent for easier grepping
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@102
PS23, Line 102: NO_PENDING_FATALS();
I don't think this has any effect - it would just return if there is a pending fatal. it needs to be after any _calls_ to this function
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@129
PS23, Line 129: // or -1 if it can't be found.
sort of odd to return -1 instead of bad status here. Up above GetTserverNumToFail would end up returning -1 in the case this returns -1 which seems not right (may lead to crash?)
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@181
PS23, Line 181: If this returns an error, the Tablet must
: // have been stopped.
Similar to some comments on earlier revs, I find this doc hard to interpret.
Is this a guarantee that: if this returns an error, then the Tablet state will have already transitioned to stopped state? In that case I think it is better to say "If this returns an error, it is guaranteed that the tablet will be in stopped state upon return."
Or, if, say, we have some storage problem in our compaction code or something, this could return an error but _not_ have the tablet in stopped state, in which case the caller should probably be CHECKing?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@190
PS23, Line 190: // have been stopped.
same as above
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@408
PS23, Line 408: // This method is not thread safe.
nit: it seems this is just copying from above, but would be nice if this clarified the thread safety a bit better. Should it be called while holding some other lock? When's it safe to call?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@465
PS23, Line 465: void set_state(State s) {
maybe name set_state_unlocked
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@466
PS23, Line 466: state_ = s;
can you add a DCHECK assert on state_lock_ here?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@594
PS23, Line 594: // ones specified by 'expected_states'.
it is surprising to me that passing an empty set here, then, has the effect that it does. Maybe this should be two separate methods?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@597
PS23, Line 597: Status CheckActiveState(const std::set<State>& expected_states = {}) const;
given that we expect the set to be very small, I think a vector is probably more efficient here than std::set
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.h@683
PS23, Line 683: potecting
typo
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@299
PS23, Line 299: &&
you mean ||?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@879
PS23, Line 879: expected_states.empty()
is this necessary?
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tablet/tablet.cc@1120
PS23, Line 1120: kOpen, kBootstrapping
how is this different than checking for {}? see other comment about maybe splitting this into separate methods that are a little easier to understand
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/tserver/tablet_service.cc@1761
PS23, Line 1761: LOG(WARNING) << "Error setting up scanner with request " << SecureShortDebugString(*req);
this should probably include s.ToString()
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 23
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 14 Nov 2017 00:08:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 39: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 39
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 22 Nov 2017 00:27:39 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 39:
Fixed IWYU + the build failure was KUDU-2222
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 39
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:40:04 +0000
Gerrit-HasComments: No
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 21:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc@108
PS21, Line 108: // Insert data until the tablet becomes visible to the server.
you don't need to insert data to get it to be visible - just creating the table should be enough to create the tablet, right?
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc@202
PS21, Line 202: // Now that the leader has been stopped, wait longer and ensure that no
: // more writes get through.
: SleepFor(MonoDelta::FromSeconds(5));
: ASSERT_EQ(num_rows_after_stop, writes.rows_inserted());
this is odd. Wouldn't you expect it to re-elect a new leader? Or is this because you're reaching into the server and calling 'Stop()' but not actually shutting down the tablet, so the leader keeps sending heartbeats? Worth a comment.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@276
PS21, Line 276: should
nit: s/should/will/
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: inline
> Is "inline" useful here? I thought methods defined in their header declarat
yea, not necessary
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: is_closed
> nit: Perhaps is_open() would be more intuitive than is_closed()? This is be
I could go either way on this. I think it's natural to talk about whether something is or is not closed (I don't interpret "closed" as a negative adjective in the same way as "disabled")
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@281
PS21, Line 281: .load()
> Can we get away with std::memory_order_relaxed?
I actually looked at the assembly generated by the default memory order, and load() just turns into a normal load instruction with nothing expensive about it, so I think this is fine as is.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@382
PS21, Line 382: closed_
> perhaps open_ and default to true?
I kinda disagree on this one since we often have this "shutdown_" type member variable.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121
PS21, Line 121: Unlike Shutdown(), this is not a lifecycle change, but rather a functional
: // one. Calls to this are orthogonal to calls to Shutdown().
> It's not clear when one vs. the other should be called. Can you clarify her
yea, agree this can still be clarified better whether it's required or optional, should be called before/after Shutdown(), etc.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@175
PS21, Line 175: error the tablet
nit: missing "if"
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@176
PS21, Line 176: It is critical for
: // transactional integrity that errors here Stop() the tablet.
this bit of the comment is confusing in this context.
General rule of thumb for header comments is that you should try to focus on what external callers need to know about the function, and be clear which things are "instructions" for the caller vs "FYI" for the caller, vs implementation details (which probably should go in the .cc instead).
In this particular example, the caller might be confused by the phrasing -- does this mean that, if this returns an error, the caller is responsible for calling Stop()? Or that if this returns an error, the tablet is guaranteed to have already stopped itself?
Structuring it as a positive statement on a post-condition may help make it clearer: "If this returns an error, the Tablet will be in stopped state." (and then if you feel it's critical to explain why this is critical for maintaining correctness, do so in the implementation?)
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@183
PS21, Line 183: // Returns an error the tablet has been stopped or if there was an error
: // (e.g. a disk failure) Applying the operations. It is critical for
: // transactional integrity that errors here Stop() the tablet.
same
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@480
PS21, Line 480: // Returns an error the tablet has been stopped or if there was an error
: // (e.g. a disk failure) when checking the keys. It is critical for
: // transactional integrity that errors here Stop() the tablet.
same
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.cc@905
PS21, Line 905: // In order for failures here to be safe, the tablet must have been
: // stopped, ensuring no further writes can commit durably.
: //
: // TODO(awong): for now, the tablet will only be Stopped() when it
: // shuts down. In the future, the tablet should be Stopped() at a lower
: // level when handling recoverable errors (e.g. a disk failures).
instead of copy-pasting this comment block a few times, maybe move this to a single spot and refer to it here?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Nov 2017 00:10:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 33:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.cc@1389
PS32, Line 1389: maintenance_ops_.swap(maintenance_ops);
: }
is this sufficient protection here for this method? My worry is that, if you call Stop() while you are in the middle of registering the ops, then the ops will still get registered, and Stop() will not cancel or unregister anything.
Or is it impossible that RegisterMaintenanceOps will end up called concurrently with a shutdown because of higher level synchronization? Maybe we should add a DFAKE_MUTEX for 'maintenace_ops_lock' and not use lock_, if we are relying on external synchronization?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 33
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 04:36:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 21:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@277
PS21, Line 277: SetClosed
nit: Close() ?
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: is_closed
nit: Perhaps is_open() would be more intuitive than is_closed()? This is because closed is a negative and checking for is not closed is a bit of a boolean trap: https://ariya.io/2011/08/hall-of-api-shame-boolean-trap
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: inline
Is "inline" useful here? I thought methods defined in their header declarations were generally inlined anyway.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@281
PS21, Line 281: .load()
Can we get away with std::memory_order_relaxed?
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@316
PS21, Line 316: CheckNotClosed
nit: CheckOpen() seems more intuitive to me per the above
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@382
PS21, Line 382: closed_
perhaps open_ and default to true?
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@98
PS21, Line 98: closed_.load()
nit: is_closed()
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@280
PS21, Line 280: did
did not
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121
PS21, Line 121: Unlike Shutdown(), this is not a lifecycle change, but rather a functional
: // one. Calls to this are orthogonal to calls to Shutdown().
It's not clear when one vs. the other should be called. Can you clarify here? Also, is it ever possible to go from Stopped() back to running? If not, this seems like a lifecycle change to me, despite the header comment here.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc@293
PS21, Line 293: tablet_->Shutdown();
We call Tablet::Shutdown() here. So why not just have Tablet::Shutdown() call Tablet::Stop()?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Oct 2017 23:00:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 21:
...flake seemed unrelated... :)
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 31 Oct 2017 02:31:44 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 36:
Just fixed the iwyu error
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 36
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:14:23 +0000
Gerrit-HasComments: No
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 12:
I like the changes in general (post specific comments in a few mins). One concern that I have in general though, is that we need to make sure that once we abort one transaction, no other changes to the tablet can be made visible. Note that I'm not saying that this is not addressed here on in follow up patches, just that I want to make sure it is.
What I mean is that if we get an error and abort a txn that was already applying, then no other applying transaction can be made visible, otherwise we might break isolation/consistency guarantees. In practice, since we're deleting the tablet data or otherwise not allowing it to be scanned I don't think it matters too much what actually gets to apply or not (i.e. I don't care too much about stopping applying transactions), but scanners should immediately be prevented from starting or continuing and copies should be prevented.
Is this taken care of?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
IWYU failed but this is ready for review.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 27 Sep 2017 23:36:45 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 27: Verified+1
Failure was caused by a seeming flake in create-table-stress-test, which didn't fail in 1000 stressed dist-test runs
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 27
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 20 Nov 2017 18:57:26 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 39: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 39
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:40:26 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#37).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); that stopped tablets don't prevent
fault-tolerant scans; and that stopping the only tablet does prevent
scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 735 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/37
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 37
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 25:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@132
PS25, Line 132: leader = i;
> why not just do: *leader_num = i; return Status::OK() here?
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@137
PS25, Line 137: return Status::NotFound("Could not find the tablet leader");
> and then just unconditionally return NotFound here if you hit the end of th
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@149
PS25, Line 149: NO_PENDING_FATALS();
> NO_FATALS(...) on the prior line already ensures that there are no pending
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@180
PS25, Line 180: NO_PENDING_FATALS();
> same
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@400
PS25, Line 400: // This method is are thread-safe.
> typo
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@590
PS25, Line 590: vector
> passing a vector here with a default-constructed argument like this seems t
Hrm, seems like constexpr initializer_lists don't work. Needs to be static const.
And this seems to be getting more complex than it needs to be, so I'm going to fall back on a macro.
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@677
PS25, Line 677: 'maintenance_ops_'.
> this is somewhat of a code smell IMO -- locks should protect data, not code
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@289
PS25, Line 289: set_state_unlocked(kBootstrapping);
> is it possible to race against a Tablet::Close() call that's concurrent wit
Done
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@313
PS25, Line 313: CHECK_EQ(state_, kBootstrapping);
> what happens if the tablet gets Stop()ed while it was in the progress of bo
I've made this return IllegalStatus() and plumbed that up to bootstrap. Failures in bootstrap leave the tablet failed.
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@1365
PS25, Line 1365: maintenance_ops_.push_back(rs_compact_op.release());
> doesn't this need to hold the lock, if someone could call a concurrent Stop
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 25
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Nov 2017 23:47:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#16).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by closing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is closed, new transactions will
abort immediately before even reaching the Prepare phase.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
closed immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is closed and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- tests in ts_tablet_manager-itest that ensure stopped leaders don't
complete writes and stopped followers do.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
13 files changed, 427 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/16
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#22).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by freezing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is frozen, new transactions will
abort immediately before even reaching the Prepare phase.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
frozen immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is frozen and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders don't
complete writes and stopped followers do, and stopped tablets don't
prevent fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
15 files changed, 507 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/22
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 22:
(20 comments)
Currently discussing an alternate (maybe new tablet state) to Stop()
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc@108
PS21, Line 108: writes.Setup();
> you don't need to insert data to get it to be visible - just creating the t
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/integration-tests/stop_tablet-itest.cc@202
PS21, Line 202: // Now that the leader has been stopped, wait longer and ensure that no
: // more writes get through.
: // Note: a new leader will not be elected because the tablet has only been
: // stopped and not shut down, so the leader can still s
> this is odd. Wouldn't you expect it to re-elect a new leader? Or is this be
Yep, exactly. The tablet is alive (this test was targeted at testing out the new Stopped state). Will add a comment.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@276
PS21, Line 276:
> nit: s/should/will/
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@277
PS21, Line 277: Close();
> nit: Close() ?
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: bool i
> yea, not necessary
Ack
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: () const
> nit: Perhaps is_open() would be more intuitive than is_closed()? This is be
Oh interesting link, thanks! I'm not sure I completely agree in this specific context, but I can see RETURN_NOT_OK(CheckNotClosed()) being confusing since it introduces a double negative. I'll try to avoid those where possible.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@280
PS21, Line 280: bool i
> Is "inline" useful here? I thought methods defined in their header declarat
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@281
PS21, Line 281: oad();
> I actually looked at the assembly generated by the default memory order, an
Will leave as is then.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@316
PS21, Line 316: CheckOpen() co
> nit: CheckOpen() seems more intuitive to me per the above
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.h@382
PS21, Line 382: open_;
> I kinda disagree on this one since we often have this "shutdown_" type memb
Hrm, interesting. Limited sample size, but it seems like for the non-init paths, the trend is to go from false-->true (e.g. various shutdown_, marking read_only_ in the LBM).
Anyway, I think I agree with Mike, at least for private APIs. It's a bit easier to follow RETURN_NOT_OK(CheckIsActive) vs RETURN_NOT_OK(CheckNotClosed).
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@98
PS21, Line 98: !is_open())) {
> nit: is_closed()
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/mvcc.cc@280
PS21, Line 280: did
> did not
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121
PS21, Line 121: Currently, the tablet will only be Stopped as the tablet is shutting down.
: // In the future, it can be Stopped when it hits a non-recov
> It's not clear when one vs. the other should be called. Can you clarify her
Ah, yeah given the current placement it does act entirely as a lifecycle change.
The idea from an error-handling POV is that Stop() can complete without holding any locks and without waiting, and as such, it can be called immediately in the presence of failures, while Shutdown() must wait for various operations to finish (it locks component_lock_). That said, Stop() is idempotent, so putting it in Shutdown() couldn't hurt.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@121
PS21, Line 121: Currently, the tablet will only be Stopped as the tablet is shutting down.
: // In the future, it can be Stopped when it hits a non-recov
> yea, agree this can still be clarified better whether it's required or opti
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@175
PS21, Line 175:
> nit: missing "if"
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@176
PS21, Line 176: this transaction.
: //
> this bit of the comment is confusing in this context.
Ah, thanks for the guidance! Done.
Same elsewhere.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@183
PS21, Line 183: // Apply a single row operation, which must already be prepared.
: // The result is set back into row_op->result.
: //
> same
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.h@480
PS21, Line 480:
: // For each of the operations in 'tx_state', check for the presence of their
: // row keys in the RowSets in the current RowSetTree (as deter
> same
Done
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet.cc@905
PS21, Line 905: if (present) {
: row_op->present_in_rowset = rowset;
: break;
: }
: }
: row_op->checked_present = true;
> instead of copy-pasting this comment block a few times, maybe move this to
Done, moved this to Stop()'s declaration.
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/7439/21/src/kudu/tablet/tablet_replica.cc@293
PS21, Line 293: / Only mark the peer
> We call Tablet::Shutdown() here. So why not just have Tablet::Shutdown() ca
Fair point. As per my other comment, I'll do this.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 01 Nov 2017 19:34:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 22:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/22/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/22/src/kudu/tablet/tablet.h@562
PS22, Line 562: CheckNotStopped
Between Stopped() and CheckNotStopped() I think we are introducing a new lifecycle state (i.e. state_) while allowing for something to be both kOpen and Stopped(). Based on that I think we should bite the bullet and find a way to transition the state outside of taking the component_lock_, perhaps by introducing a state_lock_ spinlock specifically to access state_. I'd be interested in Todd's thoughts on this, too.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 22
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 Nov 2017 01:44:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP disk failure: forced shutdown of MvccManager
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has uploaded a new patch set (#2).
Change subject: WIP disk failure: forced shutdown of MvccManager
......................................................................
WIP disk failure: forced shutdown of MvccManager
When a disk fails, tablets with data on the failed disk must shut down.
Normally, when a replica is shutdown, it must wait for all transactions
to finish. However, transactions that fail due to disk failure will not
complete.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
cannot end a transaction that is in the APPLYING state (must be
RESERVED), Cancels can end any transaction. This is only "safe" because
the tablet to whom the MvccManager belongs is expected to shutdown.
WIP because there is no testing and I'm not sure this is the best design
for what I'm trying to accomplish.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/transactions/alter_schema_transaction.h
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/tablet/transactions/transaction_tracker.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
10 files changed, 57 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/2
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS11, Line 270: d GetApplyingTransactionsTimestamps(std::vector<T
> is this precisely true? or is it that transactions are _allowed_ to abort?
I think we only _need_ to stop assigning new timestamps if the tablet's WAL fails, no? Until then, in theory, we would be able to keep writing ops to the WAL (although practically, we'd expect the tablet to eventually be shut down).
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] Allow tablet shutdown without completing txs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 11:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS11, Line 270: transactions must not durably complete operations
is this precisely true? or is it that transactions are _allowed_ to abort? there is no CHECK ensuring that, once shutting down, nothing commits, and I think adding such a restriction could be tricky.
I think it is reasonable to disallow starting of any new timestamps once shutting down, but I dont think it's doing that yet either, right?
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
PS11, Line 764: message
use ToString so we retain the original code?
Line 765: LOG(ERROR) << s.ToString();
add context of the tablet id, maybe the opid which should be available?
I think WARN is probably more appropriate because things are recovering without data loss
PS11, Line 800: exitted
typo? exited
PS11, Line 799: if (PREDICT_FALSE(mvcc_.IsShuttingDown())) {
: return Status::Aborted("Transaction exitted early because tablet is shutting down");
: }
mind making a short function like Tablet::CheckNotShuttingDown with this code in it? seems it's called from a few places
Line 852: LOG(ERROR) << "Failed to check if row is present; ending transaction early";
we should be able to CHECK(IsShuttingDown()) or something here, right?
I think we basically want an invariant that operations can only abort if the tablet is shutting down. Even if you are the one who hit the error, you need to ensure that it's marked shutting down before you complete the operation. Otherwise you could release the lock and allow some other operation to proceed and commit out-of-order.
Line 853: CHECK(s.IsDiskFailure());
I think this is a bit too restrictive. For example if there is some local heisenbug it could cause a tablet to become corrupt in some way that is not a disk failure error, and I think the correct thing to do in that circumstance is also to mark the tablet as failed (without marking all other tablets on that disk as failed)
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
Line 347: if (!state()->tablet_replica()->tablet()->mvcc_manager()->IsShuttingDown()) {
I think reaching into MvccManager is a bit strange here. Even though there is a 1:1 of tablet and mvccmanager I think it makes more sense to have the additional state enum in Tablet which says "shutting down". It happens that when the tablet shuts down, it also shuts down its MvccManager (thus allowing writes to abort without firing a CHECK) but conceptually the Tablet is the thing that is shutting down.
Also don't we already have such an enum in the TabletReplica indicating it's shutting down? it would be ideal if this code can avoid calling specifically into Tablet much.
Line 352: }
can you add FALLTHROUGH_INTENDED here?
PS11, Line 356: VLOG_WITH_PREFIX(1) << "Transaction " << ToString() << " failed prior to "
: "replication success: " << s.ToString();
this is no longer accurate in all cases
http://gerrit.cloudera.org:8080/#/c/7439/11/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
PS11, Line 640: if (replica->tablet()) {
: // If we're deleting the tablet, there's no point in ensuring that applies
: // complete. Currently-running Applies will henceforth return early.
: replica->tablet()->mvcc_manager()->SetShuttingDown();
: }
:
why not put this in TabletReplica::Shutdown() implementation itself so as to avoid spilling as many implementation details outward?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] Allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#12).
Change subject: Allow tablet shutdown without completing txs
......................................................................
Allow tablet shutdown without completing txs
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way ensure the transactions are released without crashing
the server, regardless of state.
This patch adds the ability to mark a tablet's MvccManager as shutting
down, after which Applies will exit early, responding with an Aborted
status.
Testing is done by adding the following:
- a test in mvcc-test to shut down the manager and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in tablet_replica-test to register a WriteTransaction,
shut down the tablet's MvccManager, and begin Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
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/tserver/ts_tablet_manager.cc
11 files changed, 163 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/12
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 36:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/36/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/36/src/kudu/integration-tests/stop_tablet-itest.cc@138
PS36, Line 138: TEST_P(StopTabletITest, TestStoppedTabletsScansGetRedirected) {
It seems like this works but I'm worried that if someone changes the read thread to respect timeouts_allowed later than we will lose coverage. Do you mind adding a little test that checks that with a single replica, scans fail after stopping the tablet?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 36
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:38:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] disk failure: release failed txs from tracker
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: disk failure: release failed txs from tracker
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
Line 404: void Cancel();
here's an idea: how about rather than specifically cancelling each transaction, there is an MvccManager::Shutdown() call, and then we allow AbortTransaction() of an APPLYING transaction only in the case that the MvccManager has entered shutdown state?
then we don't need new per-transaction states, and it more accurately captures the idea that "you can only cancel operations once the entire tablet ahs started shutting itself down"
(and we can still ensure that, on destruction, the map has been cleared out)
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
Line 481: CHECK(s.IsDiskFailure());
per conversation on Slack, I think generalizing this to something like the following would separate the concerns a bit better:
- on a disk error, mark the tablet as shutdown
- when attempting to apply operations on a shutdown tablet, return Status::Aborted
- if we get Status::Aborted here, we can CHECK that the tablet is in Shutdown() state and then go to the cancellation path
-- basically this would be the same code as in HandleFailure today, except widened the scope to allow cancelling a replicating/replicated transaction in the case that the tablet is in failed state? (eg by taking a new parameter to HandleFailure indicating that it's due to the tablet shutdown?)
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tablet/transactions/write_transaction.cc
File src/kudu/tablet/transactions/write_transaction.cc:
Line 134: if (PREDICT_FALSE(tablet->IsInFailedDir())) {
making ApplyRowOperations return a Status would allow centralizing this logic into Tablet a bit better and also generalizing to all "Shutdown" cases instead of just specifically the disk-failure one
Line 139: tablet->ApplyRowOperations(state());
here if Tablet indicated that it has shut down then you can early-return without creating a commit message. that would be a pretty good safety net that we don't accidentally end up writing a commit message
http://gerrit.cloudera.org:8080/#/c/7439/8/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:
Line 1076: tablet->SetInFailedDir();
again maybe generalize to just 'MarkFailed' since the handling isn't particular to the reason for the failure. (eg if we detected some internal inconsistency like a bad checksum or something we may also want to markfailed even though it's not in a failed dir)
Line 1078: LOG(FATAL) << Substitute("Data directory $0 failed. Disk failure handling not implemented",
now implemented?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 33:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h
File src/kudu/tablet/rowset_metadata.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/rowset_metadata.h@85
PS32, Line 85: Status Flush() const;
nit: it's pretty weird to mark this const, since it's technically mutating *something*, even if it goes through a const member to do that. Was this needed for some reason?
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@371
PS32, Line 371: col_ids
nit: just for future reference, doing random renames in a big patch is a bit distracting, unclear how this is particularly better, anyway...
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@668
PS32, Line 668: mutable simple_spinlock lock_;
nits:
1) Perhaps rename this state_lock_ since lock_ is so generic it sounds like the only lock in the class when that is far from the truth (there will now be 4)
2) Also, specify the lock acquisition order when / if this lock is acquired simultaneous to other locks.
3) if you have to take this lock_ to access state_ then it's better to declare it before state_ so that acquiring this lock_ probably puts state_ into the L1 cache automatically.
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.h@457
PS31, Line 457: state_ = s;
it would be nice to assert on the state transitions in this function to enforce the above DAG.
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc@320
PS31, Line 320: CancelMaintenanceOps();
Why do we need to make this call in Stop() ? Can't the ops just cancel themselves if they notice that the tablet is no longer running?
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/tablet.cc@1398
PS31, Line 1398: for (MaintenanceOp* op : maintenance_ops_) {
If we don't want to hold the lock while unregistering the ops then we need to first copy the map before dropping the lock and iterating through the copy. Or, if this method could be called simultaneously by multiple threads, we likely have to be even more careful than that.
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc@952
PS33, Line 952: s = InsertOrUpsertUnlocked(tx_state, row_op, stats);
: if (s.IsAlreadyPresent()) {
: return Status::OK();
: }
: return s;
:
: case RowOperationsPB::UPDATE:
: case RowOperationsPB::DELETE:
: s = MutateRowUnlocked(tx_state, row_op, stats);
: if (s.IsNotFound()) {
: return Status::OK();
: }
: return s;
these look like good changes... but are they at all related to the theme of the patch? Do you think we need additional test coverage or something?
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tablet/transactions/transaction_driver.cc@518
PS31, Line 518: CHECK(tablet->HasBeenStopped());
Doesn't HandleFailure already do this? As long as we document that it takes care of this then we can delegate that responsibility to that function.
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/7439/31/src/kudu/tserver/tablet_service.cc@1764
PS31, Line 1764: fall
typo: fail
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 33
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 04:11:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#34).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 681 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/34
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 34
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 35: Code-Review+1
Looks good to me. Small IWYU error and I guess Mike should also +2
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 35
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:00:16 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 36: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 36
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 19:16:59 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#36).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 676 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/36
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 36
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/mvcc.h@277
PS15, Line 277: SetShuttingDown
since there is no "action" this entity is doing maybe "closed" with be a better term?
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.h@405
PS15, Line 405: // Stops the tablet. Once called, currently-Applying operations should
: // terminate early, and further transactions should be aborted.
: void Stop();
:
: bool Stopped() const {
: return mvcc_.IsShuttingDown();
: }
I don't think we need this in tablet, since it's a plain proxy to mvcc which is already pulled out in several places
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/tablet.cc@438
PS15, Line 438: gscoped_ptr<ScopedTransaction> mvcc_tx;
: DCHECK(tx_state->has_timestamp());
: mvcc_tx.reset(new ScopedTransaction(&mvcc_, tx_state->timestamp()));
: tx_state->SetMvccTx(std::move(mvcc_tx));
I though we had discussed having transactions fail on the prepare phase if mvcc was shutdown/closed. This would be the place to do it, I think.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Sep 2017 00:00:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#15).
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
mvcc: allow tablet shutdown without completing txs
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds the ability to "stop" a tablet by shutting down its MVCC
manager. Once this happens, Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is shut down, new transactions will
abort immediately.
This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
stopped immediately, allowing currently-Applying transactions to abort
without Committing.
This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is shut down and log a
warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- tests in ts_tablet_manager-itest that ensure stopped leaders don't
complete writes and stopped followers do.
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
13 files changed, 425 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/15
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 12:
Thinking a bit more which constraint we actually want to enforce. I think we need to enforce the following one:
If a transaction with timestamp t is being aborted post-apply, then no other transaction with a timestamp t1 > t can be made visible either through scans or tablet copy.
In practice I think this might not be too hard to enforce. When applying something on the tablet that gets an ioerror we could have it immediately change the state of the mvcc manager (not have the tablet manager do so). Note that I'm just referring to the mvcc manager and not to the replica, that can shutdown later.
We could also have a "shutdown timestamp", and then make any new scans and continuing scans make sure that their scan timestamp is lower than this one.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] mvcc: allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/transactions/transaction_driver.cc
File src/kudu/tablet/transactions/transaction_driver.cc:
http://gerrit.cloudera.org:8080/#/c/7439/15/src/kudu/tablet/transactions/transaction_driver.cc@133
PS15, Line 133: TabletReplica* replica = transaction->state()->tablet_replica();
: if (PREDICT_FALSE(replica && replica->tablet()->Stopped())) {
: return Status::Aborted("Not executing transaction; tablet has been stopped");
: }
Intuitively I would say this closes the window to the scenario that was closing errors in dist test but doesn't close it completely. TBH I'm not even sure my suggestion does it. Nastier tests might be able to prove me wrong.
The reason I say it is because theres a chance that both attempts of the same txn are able to pass through here and go to prepare. It's smaller now, but it's still there, I think
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 15
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Sep 2017 00:14:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] Allow tablet shutdown without completing txs
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Allow tablet shutdown without completing txs
......................................................................
Patch Set 12:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
Line 98: if (shutdown_pending_.Load()) {
use PREDICT_FALSE
PS12, Line 99: VLOG(1)
use LOG_EVERY_N or even just LOG, this is an important message that is not too common
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
PS12, Line 275: shutdown_pending_.Store(true);
we now have std::atomic
http://gerrit.cloudera.org:8080/#/c/7439/12/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
PS12, Line 796: CHECK(mvcc_.IsShuttingDown()) << "MVCC must be shut down to survive transaction failures";
not sure I like how we're using the mvcc manager as a mediator here. seems messy. lemme think of an alternative
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 34:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/32/src/kudu/tablet/tablet.h@371
PS32, Line 371: col_ids
> This was to appease tidybot, which I suppose isn't all too important; sorry
gotcha
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/tablet/tablet.cc@952
PS33, Line 952: }
: return s;
:
: case RowOperationsPB::UPDATE:
: case RowOperationsPB::DELETE:
: s = MutateRowUnlocked(tx_state, row_op, stats);
: if (s.IsNotFound()) {
: return Status::OK();
: }
: return s;
:
: default:
: LOG_WITH_
> Hrm, somewhat, somewhat not. A part of this patch is about handling errors
Would you mind splitting these changes into their own patch? They already had ignore_result() so they couldn't fail.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 34
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 08:24:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 23:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7439/22/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/22/src/kudu/tablet/tablet.h@562
PS22, Line 562:
> Between Stopped() and CheckNotStopped() I think we are introducing a new li
Done
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 23
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 13 Nov 2017 22:51:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] disk failure: release failed txs from tracker
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#7).
Change subject: disk failure: release failed txs from tracker
......................................................................
disk failure: release failed txs from tracker
Currently, if a TransactionState object is deleted while it is Applying,
Kudu will crash as a way to prevent the failed transaction from
persisting incorrect state onto disk. However, this is not always
necessary: transactions that fail due to disk failure may want to keep
the server alive by marking the tablet as unusable. In this case, there
needs to be a way to release transactions from the TransactionTracker
without crashing Kudu, regardless of state.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
ensure the transaction is not in the APPLYING state when destructed, a
Canceled transaction has no such constraint. This is only "safe" because
the TransactionTracker's tablet is expected to shut down and no longer
be used.
Additionally, a new flag is added to Tablet to indicate that it resides
on a failed disk. If set, the tablet's transactions will end early.
Currently, this codepath will not have a visible effect on Kudu as disk
failures are still fatal, but it is tested for.
Testing is done in a couple ways:
- A test is added in mvcc-test to Cancel an Applying ScopedTransaction
and ensure that there are no errors when it leaves scope.
- A test is added to tablet_replica-test to register a WriteTransaction,
set the tablet's "in-failed-dir" flag, and being Applying. The
transaction exits early and releases itself from MVCC.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/alter_schema_transaction.h
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/transaction_tracker-test.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/ts_tablet_manager.cc
15 files changed, 133 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/7
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 20:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@130
PS20, Line 130: LOG(INFO) << "Closing replica's MVCC manager";
this should probably be less specific to implementation details
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@179
PS20, Line 179: TEST_F(TsTabletManagerITest, TestStoppedTabletsScansGetRedirected) {
I'm not sure this test suite is the best place for these tests, since they aren't really testing that much about the TsTabletManager. Maybe a new suite like tablet_shutdown-itest.cc ?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@183
PS20, Line 183: NO_FATALS(StopTablet(tablet_id));
here you're stopping a random server, not necessarily the one that the scanner(s) are reading from, right? is it worth trying to explicitly stop the leader?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@187
PS20, Line 187: reads.set_num_read_threads(1);
are the readers using fault-tolerant scans by default here?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@197
PS20, Line 197: reads.StopAndJoin();
Somewhere we have a utility function like AssertNoCrashes(). Otherwise maybe the server you stopped might have hit a crash but you wouldn't notice because the reader just failed over to another replica
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@205
PS20, Line 205: NO_FATALS(StartClusterAndLoadTablet(kNumReplicas, &tablet_id));
in these tests it might be beneficial to configure the MM such that it constantly flushes - eg set the flush threshold to 1mb? I think we have some tests elsewhere that do this.
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@235
PS20, Line 235: writes.StopAndJoin();
is it possible in this test to re-start the tserver that has the stopped tablet to make sure that it can properly replay its WAL and catch back up to the other replicas? ie to make sure that we didn't get some divergent state?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@238
PS20, Line 238: TEST_F(TsTabletManagerITest, TestStoppedFollowerTabletsDontBlockWrites) {
can you combine this test with the above one, using a utility function that takes an enum or boolean indicating whether to stop the leader or follower?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@498
PS20, Line 498: ASSERT_OK(s);
: waiting_thread.join();
don't you want to join before assert here?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@598
PS20, Line 598: ASSERT_EVENTUALLY([&] {
: std::lock_guard<simple_spinlock> l(status_lock);
: ASSERT_STR_CONTAINS(s.ToString(), "closed");
: ASSERT_TRUE(s.IsAborted());
: });
: waiting_thread.join();
:
if you reordered the join and the ASSERT then you could avoid the complexity of the lock and the ASSERT_EVENTUALLY
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@253
PS20, Line 253: an error
specify the type of status
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@254
PS20, Line 254: Status WaitForApplyingTransactionsToCommit() const;
worth adding a WARN_UNUSED_RESULT here
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@275
PS20, Line 275: now
not?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@280
PS20, Line 280: IsClosed
rename to is_closed since it's inlinable short method
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@160
PS20, Line 160: Status ApplyRowOperations(WriteTransactionState* tx_state);
mind adding a WARN_UNUSED_RESULT to these functions that you're changing from void to Status returns? That way we'll notice if we missed updating any call sites
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405
PS20, Line 405: should
nit: "should" is kinda fuzzy. Do you mean "will"? Nicer for callers to think about what guarantees this method provides and which things are more "best-effort".
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405
PS20, Line 405: // Stops the tablet. Once called, currently-Applying operations should
: // terminate early, and further transactions should be aborted.
: void Stop();
Would be good to move this declaration up next to 'Shutdown()' and clarify the comment to explain the difference between Stop and Shutdown. Do both have to be called? Which order? What's the difference?
Also should document whether this is synchronous or not (ie after this returns is there any guarantee that ops have stopped, etc?)
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@465
PS20, Line 465: Status BulkCheckPresence(WriteTransactionState* tx_state);
add doc for what bad status means. Same above with the Apply calls.
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.cc@806
PS20, Line 806: CHECK(Stopped()) << "MVCC must be closed to survive transaction failures";
I think it's worth a comment here explaining why we expect it to already have marked the tablet failed. I guess some lower layer is already handling a synchronous notification which leads back to this tablet?
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:
http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet_replica.cc@263
PS20, Line 263: if (tablet_) {
is it ok to access tablet_ outside of state_change_lock_? can't quite recall the purpose of this lock but strange to see this outside of the lock with the rest of the stuff below inside it
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 27 Oct 2017 20:14:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#39).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); that stopped tablets don't prevent
fault-tolerant scans; and that stopping the only tablet does prevent
scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
17 files changed, 733 insertions(+), 135 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/39
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 39
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
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 disk failure: forced shutdown of replicas
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#4).
Change subject: WIP disk failure: forced shutdown of replicas
......................................................................
WIP disk failure: forced shutdown of replicas
When a disk fails, replicas with data on the failed disk must shut
down. Normally, when a replica is shutdown, it must wait for all
transactions to finish. However, transactions that fail due to disk
failure will not complete.
This patch adds the ability to Cancel transactions. Unlike Aborts, which
cannot end a transaction that is in the APPLYING state (must be
RESERVED), Cancels can end any transaction. This is only "safe" because
the tablet to whom the MvccManager belongs is expected to shutdown.
In addition to this, a new tablet state is added: FAILED_AND_SHUTDOWN.
Like QUIESCING and SHUTDOWN, TabletReplica::Shutdown() can wait on
FAILED_AND_SHUTDOWN. This is needed to shut down failed tablets that
need to be replicated: calling Shutdown() cannot leave the replica in
the FAILED state, and the SHUTDOWN state cannot itself indicate the
need for a tablet copy.
WIP because there is no testing and I'm not sure this is the best design
for what I'm trying to accomplish.
This is part of a series of patches to address disk failure. To see how
this patch fits in, see section 2.3 of:
https://docs.google.com/document/d/1zZk-vb_ETKUuePcZ9ZqoSK2oPvAAaEV1sjDXes8Pxgk/edit
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/alter_schema_transaction.h
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tablet/transactions/transaction_tracker.cc
M src/kudu/tablet/transactions/transaction_tracker.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/ts_tablet_manager.cc
14 files changed, 88 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/4
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7439
to look at the new patch set (#33).
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
tablet: introduce closed mvcc and stopped tablets
Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.
This patch adds a new state to the MvccManager in which it is closed
for transactions. Once in this closed state:
1. New Applies will return and not move to the Commit phase, and any
methods waiting for the tablet's Applies to Commit (e.g. new snapshot
scans, FlushMRS) will respond with an error immediately. This allows
an escape from the existing invariant that Applies _must_ Commit,
provided the MvccManager is in this closed state.
2. Applies that are already underway may still Commit, but will return
early on a best-effort basis. These non-Committed operations are
inconsequential w.r.t. consistency; having some in-flight
transactions Commit and others not is consistent with the server
shutting down in between the Commits of two transactions.
3. New transactions drivers will abort immediately before even reaching
the Prepare phase, ensuring no more writes to the tablet are made
durable.
The Tablet class uses this closed MVCC state in a new "stopped" state of
its own. A Tablet that has been stopped will avoid further activity:
its MvccManager is closed to prevent further writes, and its maintenance
ops are cancelled to prevent further scheduling.
This patch includes these new behaviors when shutting down a tablet,
with the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing or its further maintenance ops. Code paths that previously
crashed if Applies did not succeed (e.g. TransactionDriver::ApplyTask,
MvccManager::AbortTransaction, etc.) or that waited for Applies to
finish (e.g. Tablet:: FlushUnlocked) will now _not_ crash if the Tablet
has been stopped and will log a warning instead.
Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders block
writes (because they cannot start new transactions) and stopped
followers don't (because while they cannot service the op, there still
exists a majority that can); and that stopped tablets don't prevent
fault-tolerant scans
Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset_metadata.cc
M src/kudu/tablet/rowset_metadata.h
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
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/tserver/tablet_service.cc
19 files changed, 645 insertions(+), 138 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/33
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 33
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 37:
(1 comment)
looks good
http://gerrit.cloudera.org:8080/#/c/7439/37/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/37/src/kudu/integration-tests/stop_tablet-itest.cc@193
PS37, Line 193: ASSERT_TRUE(s.IsTimedOut())
nit: add << s.ToString() so this is diagnosable when / if it fails in the future.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 37
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 22:02:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] introduce closed mvcc and stopped tablets
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 25:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/23/src/kudu/integration-tests/stop_tablet-itest.cc@90
PS23, Line 90:
> Hrm, looked around and it seems we mostly use "T xxx P xxx"? That's how it
oh, you're right, not sure what I was thinking, sorry
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@132
PS25, Line 132: leader = i;
why not just do: *leader_num = i; return Status::OK() here?
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@137
PS25, Line 137: return Status::NotFound("Could not find the tablet leader");
and then just unconditionally return NotFound here if you hit the end of the loop?
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@149
PS25, Line 149: NO_PENDING_FATALS();
NO_FATALS(...) on the prior line already ensures that there are no pending fatals after its expression is evaluated
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/integration-tests/stop_tablet-itest.cc@180
PS25, Line 180: NO_PENDING_FATALS();
same
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@400
PS25, Line 400: // This method is are thread-safe.
typo
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@590
PS25, Line 590: vector
passing a vector here with a default-constructed argument like this seems to generate a new/delete pair when you call it, even with the default argument. Since this is called relatively freqently we should avoid this.
I think using std::initializer_list<State> would avoid it (see https://godbolt.org/g/6HWFE6 )
I also still think this is a confusing name, because I read it as a synonym of "CheckCurrentState" and therefore it's somewhat surprising to me what the behavior is.
What if you instead did something like:
constexpr std::initializer_list<State> kActiveStates = {kStarting, kBootstrapping, ...];
// Check the current state of the tablet. If it is one of the allowed states, returns OK. If it is one of the
// FATAL states, exits the process with a fatal error. Otherwise returns IllegalState.
CheckState(std::initializer_list<State> allowed_states, std::initializer_list<State> fatal_states)
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.h@677
PS25, Line 677: 'maintenance_ops_'.
this is somewhat of a code smell IMO -- locks should protect data, not code. Should this really just say that it protects 'maintenance_ops_ and state_'? and then we should also take this in RegisterMaintenanceOps?
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@289
PS25, Line 289: set_state_unlocked(kBootstrapping);
is it possible to race against a Tablet::Close() call that's concurrent with the above section? In that case, would this then "overwrite" the closed state?
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@313
PS25, Line 313: CHECK_EQ(state_, kBootstrapping);
what happens if the tablet gets Stop()ed while it was in the progress of bootstrapping? then wouldn't it crash upon completion of the bootstrap?
http://gerrit.cloudera.org:8080/#/c/7439/25/src/kudu/tablet/tablet.cc@1365
PS25, Line 1365: maintenance_ops_.push_back(rs_compact_op.release());
doesn't this need to hold the lock, if someone could call a concurrent Stop() with this method?
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 25
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Nov 2017 01:07:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 20:
Have been updating to fix lint / tidybot / iwyu.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 20
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 26 Oct 2017 06:32:11 +0000
Gerrit-HasComments: No
[kudu-CR] tablet: introduce closed mvcc and stopped tablets
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: tablet: introduce closed mvcc and stopped tablets
......................................................................
Patch Set 33:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc
File src/kudu/integration-tests/stop_tablet-itest.cc:
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@137
PS33, Line 137: // Ensure that stopping a tablet will not prevent scans from completing.
I don't really understand what is being tested here. With 3 replicas, what guarantees do we have that the replica being scanned is the one we're stopping?
http://gerrit.cloudera.org:8080/#/c/7439/33/src/kudu/integration-tests/stop_tablet-itest.cc@200
PS33, Line 200: a new leader will not be elected
This will likely be flaky because under load, IO blips can cause new leaders to be elected even when the leader is not truly offline.
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 33
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 04:25:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 )
Change subject: mvcc: allow tablet shutdown without completing txs
......................................................................
Patch Set 16:
What's changed: rebased and renamed mvcc.shutting_down to mvcc.closed
--
To view, visit http://gerrit.cloudera.org:8080/7439
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 25 Oct 2017 22:43:54 +0000
Gerrit-HasComments: No