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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
