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 <[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
