Skip to content

Commit

Permalink
[#5789, #5783, #5755] Don't cleanup transaction that potentially was …
Browse files Browse the repository at this point in the history
…committed

Summary:
Since commit could be retried we could get into situation when transaction was actually committed, but during commit transaction reported as aborted.
So we should not cleanup intents for such transactions, because it could delete committed transaction intents from participant.

Also this diff relaxes requirements in CqlIndexTest.TxnCleanup, so tablet could contain small amount of running transactions.

Test Plan:
ybd --gtest_filter CqlIndexTest.TxnCleanup
ybd tsan --gtest_filter SnapshotTxnTest.MultiWriteWithRestartAndLongApply -n 20

Reviewers: mikhail, timur

Reviewed By: timur

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D9440
  • Loading branch information
spolitov committed Sep 23, 2020
1 parent 6423c5e commit a508b63
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 10 deletions.
9 changes: 7 additions & 2 deletions src/yb/client/snapshot-txn-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,7 @@ bool IntermittentTxnFailure(const Status& status) {
"Transaction expired"s,
"Transaction metadata missing"s,
"Unknown transaction, could be recently aborted"s,
"Transaction was recently aborted"s,
};
auto msg = status.ToString();
for (const auto& allowed : kAllowedMessages) {
Expand Down Expand Up @@ -685,10 +686,14 @@ void SnapshotTxnTest::TestMultiWriteWithRestart() {
op = ReadRow(session, key->value);
auto flush_result = session->Flush();
if (flush_result.ok()) {
break;
if (op->succeeded()) {
break;
}
if (op->response().error_message().find("timed out after") == std::string::npos) {
ASSERT_TRUE(op->succeeded()) << "Read failed: " << op->response().ShortDebugString();
}
}
}
ASSERT_TRUE(op->succeeded()) << "Read failed: " << op->response().ShortDebugString();
auto rowblock = yb::ql::RowsResult(op.get()).GetRowBlock();
ASSERT_EQ(rowblock->row_count(), 1);
const auto& first_column = rowblock->row(0).column(0);
Expand Down
4 changes: 0 additions & 4 deletions src/yb/client/transaction.cc
Original file line number Diff line number Diff line change
Expand Up @@ -712,10 +712,6 @@ class YBTransaction::Impl final {
}
VLOG_WITH_PREFIX(4) << "Commit done: " << actual_status;
commit_callback_(actual_status);

if (actual_status.IsExpired()) {
DoAbortCleanup(transaction);
}
}

void AbortDone(const Status& status,
Expand Down
2 changes: 1 addition & 1 deletion src/yb/integration-tests/cql-index-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ TEST_F(CqlIndexTest, TxnCleanup) {

CleanFutures(&futures, CheckReady::kFalse);

AssertNoRunningTransactions(cluster_.get());
AssertRunningTransactionsCountLessOrEqualTo(cluster_.get(), 5);
}

} // namespace yb
10 changes: 7 additions & 3 deletions src/yb/integration-tests/mini_cluster_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ size_t CountRunningTransactions(MiniCluster* cluster) {
return result;
}

void AssertNoRunningTransactions(MiniCluster* cluster) {
void AssertRunningTransactionsCountLessOrEqualTo(MiniCluster* cluster, size_t limit_per_tablet) {
MonoTime deadline = MonoTime::Now() + 7s * kTimeMultiplier;
bool has_bad = false;
for (int i = 0; i != cluster->num_tablet_servers(); ++i) {
Expand Down Expand Up @@ -67,8 +67,8 @@ void AssertNoRunningTransactions(MiniCluster* cluster) {
for (const auto& peer : tablets) {
auto participant = peer->tablet()->transaction_participant();
if (participant) {
auto status = Wait([participant] {
return participant->TEST_GetNumRunningTransactions() == 0;
auto status = Wait([participant, limit_per_tablet] {
return participant->TEST_GetNumRunningTransactions() <= limit_per_tablet;
},
deadline,
"Wait until no transactions are running");
Expand All @@ -85,4 +85,8 @@ void AssertNoRunningTransactions(MiniCluster* cluster) {
ASSERT_FALSE(has_bad);
}

void AssertNoRunningTransactions(MiniCluster* cluster) {
AssertRunningTransactionsCountLessOrEqualTo(cluster, 0);
}

} // namespace yb
1 change: 1 addition & 0 deletions src/yb/integration-tests/mini_cluster_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class MiniCluster;

size_t CountRunningTransactions(MiniCluster* cluster);
void AssertNoRunningTransactions(MiniCluster* cluster);
void AssertRunningTransactionsCountLessOrEqualTo(MiniCluster* cluster, size_t limit_per_tablet);

} // namespace yb

Expand Down

0 comments on commit a508b63

Please sign in to comment.