You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/11/15 03:56:31 UTC
[kudu-CR] WIP: [integration tests] scan inconsistency test
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5084
Change subject: WIP: [integration tests] scan inconsistency test
......................................................................
WIP: [integration tests] scan inconsistency test
This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.
This is in the context of KUDU-1679 Propagate timestamps for scans.
Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistent-scan-test.cc
2 files changed, 258 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5084/1
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [integration tests] added scan consistency test
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [integration tests] added scan consistency test
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5084/4/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
Line 343: const size_t total_rows = 2U * key_split_value_;
> warning: either cast from 'unsigned int' to 'const size_t' (aka 'const unsi
tidy bot still complaining :P maybe avoid using size_t?
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5084
to look at the new patch set (#4).
Change subject: [integration tests] added scan consistency test
......................................................................
[integration tests] added scan consistency test
This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.
This is in the context of KUDU-1679 Propagate timestamps for scans.
Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 401 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5084/4
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: [integration tests] scan inconsistency test
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP: [integration tests] scan inconsistency test
......................................................................
Patch Set 1:
Maybe rename this to 'consistency-itest' or somesuch so that it provides a useful home for all consistency-related integration tests, scan and otherwise?
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5084
to look at the new patch set (#3).
Change subject: [integration tests] added scan consistency test
......................................................................
[integration tests] added scan consistency test
This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.
This is in the context of KUDU-1679 Propagate timestamps for scans.
Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 401 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5084/3
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [integration tests] added scan consistency test
......................................................................
Patch Set 2:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.cc
File src/kudu/client/client-test-util.cc:
Line 21:
> extra line and include order
This is intentional to conform with our C++ style guide: https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
Line 78: row_count_status = scanner->Open();
> if scanner->NextBatch() on LOC 89 timesout won't you Open() a scanner that'
Good catch! Yes, that I will fix.
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:
PS2, Line 55: CountTableRows
> Maybe call this CountTableRowsWithRetries or something? I would also not be
Yes, I think it's worth updating the name.
Good -- that's another chance for small re-factoring. I will think of doing that in a separate changelist along with putting some methods introduced here for re-use by other tests.
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
Line 59: using kudu::master::TableInfo;
> warning: using decl 'TableInfo' is unused [misc-unused-using-decls]
Done
Line 60: using kudu::master::TabletInfo;
> warning: using decl 'TabletInfo' is unused [misc-unused-using-decls]
Done
Line 62: using kudu::tablet::Tablet;
> warning: using decl 'Tablet' is unused [misc-unused-using-decls]
Done
Line 68: using std::thread;
> warning: using decl 'thread' is unused [misc-unused-using-decls]
Done
PS2, Line 90: FLAGS_heartbeat_interval_ms = 10;
> why?
I copied this from client-test. This is for faster tests: if something goes wrong, with this faster setting it reports about an error faster, and overall about 700ms per test.
I'll add a comment.
Line 116: shared_ptr<KuduTable>* table) {
> warning: parameter 'table' is unused [misc-unused-parameters]
Done
PS2, Line 130: unique_ptr<KuduInsert> BuildTestRow(KuduTable* table, int index) {
: unique_ptr<KuduInsert> insert(table->NewInsert());
: KuduPartialRow* row = insert->mutable_row();
: CHECK_OK(row->SetInt32(0, index));
: CHECK_OK(row->SetInt32(1, index * 2));
: CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello %d", index))));
: return insert;
: }
:
: // Inserts given number of tests rows into the default test table
: // in the context of the specified session.
: Status InsertTestRows(KuduClient* client, KuduTable* table,
: int num_rows, int first_row = 0) {
: shared_ptr<KuduSession> session = client->NewSession();
: RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
: session->SetTimeoutMillis(60000);
: for (int i = first_row; i < num_rows + first_row; ++i) {
: unique_ptr<KuduInsert> insert(BuildTestRow(table, i));
: RETURN_NOT_OK(session->Apply(insert.release()));
: }
: RETURN_NOT_OK(session->Flush());
: return Status::OK();
: }
:
: Status GetRowCount(KuduTable* table, KuduScanner::ReadMode read_mode,
: uint64_t ts, size_t* row_count) {
: KuduScanner scanner(table);
: RETURN_NOT_OK(scanner.SetReadMode(read_mode));
: if (read_mode == KuduScanner::READ_AT_SNAPSHOT && ts != 0) {
: RETURN_NOT_OK(scanner.SetSnapshotRaw(ts + 1));
: }
: RETURN_NOT_OK(CountTableRows(&scanner, row_count));
: return Status::OK();
: }
:
: Status GetTabletIdForKeyValue(int32_t key_value_begin,
: int32_t key_value_end,
: const string& table_name,
: vector<string>* tablet_ids) {
: if (!tablet_ids) {
: return Status::InvalidArgument("null output container");
: }
: tablet_ids->clear();
:
: // Find the tablet for the first range (i.e. for the rows to be inserted).
: unique_ptr<KuduPartialRow> split_row_start(schema_.NewRow());
: RETURN_NOT_OK(split_row_start->SetInt32(0, key_value_begin));
: string partition_key_start;
: RETURN_NOT_OK(split_row_start->EncodeRowKey(&partition_key_start));
:
: unique_ptr<KuduPartialRow> split_row_end(schema_.NewRow());
: RETURN_NOT_OK(split_row_end->SetInt32(0, key_value_end));
: string partition_key_end;
: RETURN_NOT_OK(split_row_end->EncodeRowKey(&partition_key_end));
:
: GetTableLocationsRequestPB req;
: req.mutable_table()->set_table_name(table_name);
: req.set_partition_key_start(partition_key_start);
: req.set_partition_key_end(partition_key_end);
: master::CatalogManager* catalog =
: cluster_->mini_master()->master()->catalog_manager();
: GetTableLocationsResponsePB resp;
: CatalogManager::ScopedLeaderSharedLock l(catalog);
: RETURN_NOT_OK(l.first_failed_status());
: RETURN_NOT_OK(catalog->GetTableLocations(&req, &resp));
: for (size_t i = 0; i < resp.tablet_locations_size(); ++i) {
: tablet_ids->emplace_back(resp.tablet_locations(i).tablet_id());
: }
:
: return Status::OK();
: }
> any way to re-use some stuff from other tests? Maybe add some stuff to Test
I think BuildTestRow() is specific to the schema used by particular test; the same for InsertTestRows().
GetTabletIdForKeyVlue() is bound to the fact that the key is a single row and of int32_t type and the test is using MiniCluster setup. That I would move into MiniCluster base or something like that.
Let me do that in a separate changelist -- I think it would be a chance for small re-factoring as well.
PS2, Line 256: snapshot read
> I see that below you're using READ_LATEST scans and not READ_AT_SNAPSHOT. I
Good catch! Frankly, I just put it that way when I was iterating on the patch and the things were not working as I expected. Let me add those comments.
PS2, Line 296: uint64_t ts;
> maybe take the last observed timestamp from the write above too? and make s
Sure -- good idea.
Line 326: //ASSERT_OK(GetRowCount(table.get(), KuduScanner::READ_LATEST, ts,
> leftover stuff to remove?
Done
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: [integration tests] scan inconsistency test
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: WIP: [integration tests] scan inconsistency test
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5084/1/src/kudu/integration-tests/consistent-scan-test.cc
File src/kudu/integration-tests/consistent-scan-test.cc:
PS1, Line 48: class ConsistentScanTest : public KuduTest {
It would be great to have a way to have each specific test start a cluster a certain way. For certain tests multiple tablets of one replica are ideal, for other we need a single tablet with multiple replicas. A helper method like StartCluster(int num_tablets, int num_replicas) would be helpful.
PS1, Line 48: ConsistentScanTest
Like we had discussed yesterday and like Todd suggested ConsistencyITest would be a better name for the class/test.
PS1, Line 80: ExternalMiniClusterOptions
In most of the cases I can think of we need a MiniCluster, not an ExternalMIniCluster, so that we can skew the clock. Maybe worth pondering if we'll need a test that stops/kills servers in which case having the option to choose between the two would be best.
PS1, Line 84: // Creates a table with the specified name and replication factor.
: void CreateTable(const string& table_name, int num_replicas,
: shared_ptr<KuduTable>* table) {
: // Using range partitions with high split value for the key -- this is
: // to keep the contents of the table primarily at one tablet server.
: unique_ptr<KuduPartialRow> split_row(schema_.NewRow());
: ASSERT_OK(split_row->SetInt32(0, 8));
:
: unique_ptr<KuduTableCreator> table_creator(client_->NewTableCreator());
: ASSERT_OK(table_creator->table_name(table_name)
: .schema(&schema_)
: .add_range_partition_split(split_row.release())
: .set_range_partition_columns({ key_column_name_ })
: .num_replicas(num_replicas)
: .Create());
: ASSERT_OK(client_->OpenTable(table_name, table));
: }
Worth looking into other itests to see if we can reuse some of the setup/helpers there. TabletHistoryGcITest or RaftConsensusITest come to mind
PS1, Line 102: unique_ptr<KuduInsert> BuildTestRow(KuduTable* table, int index) {
: unique_ptr<KuduInsert> insert(table->NewInsert());
: KuduPartialRow* row = insert->mutable_row();
: CHECK_OK(row->SetInt32(0, index));
: CHECK_OK(row->SetInt32(1, index * 2));
: CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello %d", index))));
: return insert;
: }
:
: // Inserts given number of tests rows into the default test table
: // in the context of the specified session.
: Status InsertTestRows(KuduSession* session, InsertFlushOptions flush_opt,
: int num_rows, int first_row = 0) {
: RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
: session->SetTimeoutMillis(60000);
: for (int i = first_row; i < num_rows + first_row; ++i) {
: unique_ptr<KuduInsert> insert(BuildTestRow(table_.get(), i));
: RETURN_NOT_OK(session->Apply(insert.release()));
: }
: if (flush_opt == OPT_FLUSH) {
: RETURN_NOT_OK(session->Flush());
: }
: return Status::OK();
: }
:
: Status GetRowCount(KuduClient::ReplicaSelection replica_selection,
: KuduScanner::ReadMode read_mode,
: size_t* row_count) {
: KuduScanner scanner(table_.get());
: RETURN_NOT_OK(scanner.SetReadMode(read_mode));
: RETURN_NOT_OK(scanner.SetSelection(replica_selection));
: // KUDU-1656: there might be timeouts, so re-try the operations to
: // avoid test flakiness.
: Status row_count_status;
: size_t actual_row_count = 0;
: for (size_t i = 0; i < 3; ++i) {
: row_count_status = scanner.Open();
: if (!row_count_status.ok()) {
: if (row_count_status.IsTimedOut()) {
: // Start the row count over again.
: continue;
: }
: RETURN_NOT_OK(row_count_status);
: }
: size_t count = 0;
: while (scanner.HasMoreRows()) {
: KuduScanBatch batch;
: row_count_status = scanner.NextBatch(&batch);
: if (!row_count_status.ok()) {
: if (row_count_status.IsTimedOut()) {
: // Break the NextBatch() cycle and start row count over again.
: break;
: }
: RETURN_NOT_OK(row_count_status);
: }
: count += batch.NumRows();
: }
: if (row_count_status.ok()) {
: // Success: stop the retry cycle.
: actual_row_count = count;
: break;
: }
: }
: RETURN_NOT_OK(row_count_status);
: if (row_count) {
: *row_count = actual_row_count;
: }
: return Status::OK();
: }
same
PS1, Line 193: TEST_F(ConsistentScanTest, DISABLED_TwoReadsAfterWrite) {
I think the cleanest to test scan response timestamp propagation is to do the following:
- Start two mini cluster tservers and create a table with two tablets ({a, b} order matters), single replica.
- Set the flag to use the mock wall clock.
- Advance the clock in tablet a's tserver by some amount
- Write a row to tablet a, discard the client.
- With a new client, read the row from tablet a (snapshot read, no assigned timestamp), then write a row to tablet b. Take note of write timestamp. Discard the client.
Now Scan both tablets at the timestamp of the write to tablet b. Since b followed a (quite literally, we scanned a before writing b). We should see both rows, but because we didn't propagate the timestamp from a's scan to b's write that shouldn't be the case.
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [integration tests] added scan consistency test
......................................................................
[integration tests] added scan consistency test
This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.
This is in the context of KUDU-1679 Propagate timestamps for scans.
Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Reviewed-on: http://gerrit.cloudera.org:8080/5084
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 401 insertions(+), 0 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: [integration tests] scan inconsistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP: [integration tests] scan inconsistency test
......................................................................
Patch Set 1:
> Maybe rename this to 'consistency-itest' or somesuch so that it
> provides a useful home for all consistency-related integration
> tests, scan and otherwise?
Good idea, will change to consistency-itest.
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [integration tests] added scan consistency test
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [integration tests] added scan consistency test
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.cc
File src/kudu/client/client-test-util.cc:
Line 21:
extra line and include order
Line 78: row_count_status = scanner->Open();
if scanner->NextBatch() on LOC 89 timesout won't you Open() a scanner that's already open here?
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:
PS2, Line 55: CountTableRows
Maybe call this CountTableRowsWithRetries or something? I would also not be averse to making this one the only version.
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS2, Line 90: FLAGS_heartbeat_interval_ms = 10;
why?
PS2, Line 130: unique_ptr<KuduInsert> BuildTestRow(KuduTable* table, int index) {
: unique_ptr<KuduInsert> insert(table->NewInsert());
: KuduPartialRow* row = insert->mutable_row();
: CHECK_OK(row->SetInt32(0, index));
: CHECK_OK(row->SetInt32(1, index * 2));
: CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello %d", index))));
: return insert;
: }
:
: // Inserts given number of tests rows into the default test table
: // in the context of the specified session.
: Status InsertTestRows(KuduClient* client, KuduTable* table,
: int num_rows, int first_row = 0) {
: shared_ptr<KuduSession> session = client->NewSession();
: RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
: session->SetTimeoutMillis(60000);
: for (int i = first_row; i < num_rows + first_row; ++i) {
: unique_ptr<KuduInsert> insert(BuildTestRow(table, i));
: RETURN_NOT_OK(session->Apply(insert.release()));
: }
: RETURN_NOT_OK(session->Flush());
: return Status::OK();
: }
:
: Status GetRowCount(KuduTable* table, KuduScanner::ReadMode read_mode,
: uint64_t ts, size_t* row_count) {
: KuduScanner scanner(table);
: RETURN_NOT_OK(scanner.SetReadMode(read_mode));
: if (read_mode == KuduScanner::READ_AT_SNAPSHOT && ts != 0) {
: RETURN_NOT_OK(scanner.SetSnapshotRaw(ts + 1));
: }
: RETURN_NOT_OK(CountTableRows(&scanner, row_count));
: return Status::OK();
: }
:
: Status GetTabletIdForKeyValue(int32_t key_value_begin,
: int32_t key_value_end,
: const string& table_name,
: vector<string>* tablet_ids) {
: if (!tablet_ids) {
: return Status::InvalidArgument("null output container");
: }
: tablet_ids->clear();
:
: // Find the tablet for the first range (i.e. for the rows to be inserted).
: unique_ptr<KuduPartialRow> split_row_start(schema_.NewRow());
: RETURN_NOT_OK(split_row_start->SetInt32(0, key_value_begin));
: string partition_key_start;
: RETURN_NOT_OK(split_row_start->EncodeRowKey(&partition_key_start));
:
: unique_ptr<KuduPartialRow> split_row_end(schema_.NewRow());
: RETURN_NOT_OK(split_row_end->SetInt32(0, key_value_end));
: string partition_key_end;
: RETURN_NOT_OK(split_row_end->EncodeRowKey(&partition_key_end));
:
: GetTableLocationsRequestPB req;
: req.mutable_table()->set_table_name(table_name);
: req.set_partition_key_start(partition_key_start);
: req.set_partition_key_end(partition_key_end);
: master::CatalogManager* catalog =
: cluster_->mini_master()->master()->catalog_manager();
: GetTableLocationsResponsePB resp;
: CatalogManager::ScopedLeaderSharedLock l(catalog);
: RETURN_NOT_OK(l.first_failed_status());
: RETURN_NOT_OK(catalog->GetTableLocations(&req, &resp));
: for (size_t i = 0; i < resp.tablet_locations_size(); ++i) {
: tablet_ids->emplace_back(resp.tablet_locations(i).tablet_id());
: }
:
: return Status::OK();
: }
any way to re-use some stuff from other tests? Maybe add some stuff to TestWorkload to be able to write single rows?
PS2, Line 256: snapshot read
I see that below you're using READ_LATEST scans and not READ_AT_SNAPSHOT. I guess this is because: 1) we're incorrectly setting the last observed timestamp on the client to the scan's snapshot timestamp and 2) READ_AT_SNAPSHOT waits for the clock to advance, which in this case is only done manually.
I think to do READ_LATEST scans is ok though, but then maybe change the comment and explain why?
PS2, Line 296: uint64_t ts;
maybe take the last observed timestamp from the write above too? and make sure that the timestamp of the second write is higher? This should fail so maybe use EXPECT and not ASSERT
Line 326: //ASSERT_OK(GetRowCount(table.get(), KuduScanner::READ_LATEST, ts,
leftover stuff to remove?
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [integration tests] added scan consistency test
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5084/4/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
Line 343: const size_t total_rows = 2U * key_split_value_;
> tidy bot still complaining :P maybe avoid using size_t?
it's a typo: there should have been 2UL * ...
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [integration tests] added scan consistency test
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [integration tests] added scan consistency test
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5084
to look at the new patch set (#5).
Change subject: [integration tests] added scan consistency test
......................................................................
[integration tests] added scan consistency test
This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.
This is in the context of KUDU-1679 Propagate timestamps for scans.
Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 401 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5084/5
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [integration tests] added scan consistency test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5084
to look at the new patch set (#2).
Change subject: [integration tests] added scan consistency test
......................................................................
[integration tests] added scan consistency test
This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.
This is in the context of KUDU-1679 Propagate timestamps for scans.
Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 386 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/5084/2
--
To view, visit http://gerrit.cloudera.org:8080/5084
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>