Repository: kudu Updated Branches: refs/heads/master 0a4541bba -> 7b01d8132
KUDU-1760. Add test coverage of reading pre-REINSERT after ALTER This adds a test for the case where an ALTER TABLE adds a column, and then we read the old history of that row from prior to the addition of the column. We make sure that the default value of the new column is returned. Change-Id: Id7d44f6e2fe4d3cae03e0024c70e9f2ae84b614c Reviewed-on: http://gerrit.cloudera.org:8080/5488 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/5d12bdb9 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/5d12bdb9 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/5d12bdb9 Branch: refs/heads/master Commit: 5d12bdb904388f0e6a50b41b60138f3dff2efe00 Parents: 0a4541b Author: Todd Lipcon <[email protected]> Authored: Tue Dec 13 14:42:38 2016 +0700 Committer: Todd Lipcon <[email protected]> Committed: Thu Jan 26 21:15:01 2017 +0000 ---------------------------------------------------------------------- src/kudu/integration-tests/alter_table-test.cc | 47 +++++++++++++++++++++ src/kudu/tablet/compaction.cc | 1 - 2 files changed, 47 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/5d12bdb9/src/kudu/integration-tests/alter_table-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc index 56c0e7b..144c48a 100644 --- a/src/kudu/integration-tests/alter_table-test.cc +++ b/src/kudu/integration-tests/alter_table-test.cc @@ -53,6 +53,7 @@ DECLARE_bool(enable_maintenance_manager); DECLARE_int32(heartbeat_interval_ms); DECLARE_int32(flush_threshold_mb); DECLARE_bool(use_hybrid_clock); +DECLARE_bool(scanner_allow_snapshot_scans_with_logical_timestamps); namespace kudu { @@ -60,6 +61,7 @@ using client::CountTableRows; using client::KuduClient; using client::KuduClientBuilder; using client::KuduColumnSchema; +using client::KuduDelete; using client::KuduError; using client::KuduInsert; using client::KuduRowResult; @@ -95,6 +97,7 @@ class AlterTableTest : public KuduTest { CHECK_OK(b.Build(&schema_)); FLAGS_use_hybrid_clock = false; + FLAGS_scanner_allow_snapshot_scans_with_logical_timestamps = true; } void SetUp() override { @@ -208,6 +211,7 @@ class AlterTableTest : public KuduTest { void VerifyRows(int start_row, int num_rows, VerifyPattern pattern); void InsertRows(int start_row, int num_rows); + void DeleteRow(int row_key); Status InsertRowsSequential(const string& table_name, int start_row, int num_rows); @@ -451,6 +455,19 @@ void AlterTableTest::InsertRows(int start_row, int num_rows) { FlushSessionOrDie(session); } +void AlterTableTest::DeleteRow(int row_key) { + shared_ptr<KuduSession> session = client_->NewSession(); + shared_ptr<KuduTable> table; + CHECK_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH)); + session->SetTimeoutMillis(15 * 1000); + CHECK_OK(client_->OpenTable(kTableName, &table)); + + unique_ptr<KuduDelete> del(table->NewDelete()); + CHECK_OK(del->mutable_row()->SetInt32(0, bswap_32(row_key))); + CHECK_OK(session->Apply(del.release())); + FlushSessionOrDie(session); +} + Status AlterTableTest::InsertRowsSequential(const string& table_name, int start_row, int num_rows) { shared_ptr<KuduSession> session = client_->NewSession(); shared_ptr<KuduTable> table; @@ -852,6 +869,36 @@ TEST_F(AlterTableTest, TestMajorCompactDeltasAfterAddUpdateRemoveColumn) { JoinStrings(rows, "\n")); } +// Test that, if we have history of previous versions of a row stored as REINSERTs, +// and those REINSERTs were written prior to adding a new column, that reading the +// old versions of the row will return the default value for the new column. +// See KUDU-1760. +TEST_F(AlterTableTest, TestReadHistoryAfterAlter) { + FLAGS_enable_maintenance_manager = false; + + InsertRows(0, 1); + DeleteRow(0); + InsertRows(0, 1); + DeleteRow(0); + InsertRows(0, 1); + uint64_t ts1 = client_->GetLatestObservedTimestamp(); + ASSERT_OK(tablet_peer_->tablet()->Flush()); + ASSERT_OK(AddNewI32Column(kTableName, "c2", 12345)); + + shared_ptr<KuduTable> table; + ASSERT_OK(client_->OpenTable(kTableName, &table)); + KuduScanner scanner(table.get()); + ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT)); + // TODO(KUDU-1813): why do we need the '- 1' below? this seems odd that the + // "latest observed" timestamp is one higher than the thing we wrote, and the + // Delete then is assigned that timestamp. + ASSERT_OK(scanner.SetSnapshotRaw(ts1 - 1)); + vector<string> row_strings; + client::ScanToStrings(&scanner, &row_strings); + ASSERT_EQ(1, row_strings.size()); + ASSERT_EQ("(int32 c0=0, int32 c1=0, int32 c2=12345)", row_strings[0]); +} + // Thread which inserts rows into the table. // After each batch of rows is inserted, next_idx_ is updated // to communicate how much data has been written (and should now be http://git-wip-us.apache.org/repos/asf/kudu/blob/5d12bdb9/src/kudu/tablet/compaction.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tablet/compaction.cc b/src/kudu/tablet/compaction.cc index 45f5772..afe4b88 100644 --- a/src/kudu/tablet/compaction.cc +++ b/src/kudu/tablet/compaction.cc @@ -1016,7 +1016,6 @@ Status ApplyMutationsAndGenerateUndos(const MvccSnapshot& snap, // 2 - Apply the changes of the reinsert to the latest version of the row // capturing the old row while we're at it. - // TODO(dralves) Make Reinserts set defaults on the dest row. See KUDU-1760. undo_encoder.SetToReinsert(); RETURN_NOT_OK_LOG(redo_decoder.MutateRowAndCaptureChanges( dst_row, static_cast<Arena*>(nullptr), &undo_encoder), ERROR,
