Adar Dembo has posted comments on this change. Change subject: [c++client] fixed KUDU-1743 ......................................................................
Patch Set 1: (14 comments) http://gerrit.cloudera.org:8080/#/c/5048/1/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: PS1, Line 745: // Remove all the ops from the "in-flight" list. It's essential to do so : // _after_ adding all error into the collector, see KUDU-1743. It's good to link to the JIRA, but can you also provide an example to show how that could happen? http://gerrit.cloudera.org:8080/#/c/5048/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: PS1, Line 277: for (int i = first_row; i < num_rows + first_row; i++) { : gscoped_ptr<KuduInsert> insert(BuildTestRow(table, i)); : ASSERT_OK(session->Apply(insert.release())); : } This can now be chained into the new InsertTestRows() variant. PS1, Line 2481: returned earlier : // than the corresponding errors have gotten into the error collector. "returned before the corresponding errors were added to the error collector" Line 2488: class CustomErrorCollector : public ErrorCollector { Another option is to use MAYBE_INJECT_RANDOM_LATENCY() in the regular error collector (or in ProcessWriteResponse()). It dirties the production code a bit but obviates the need for a custom error collector. Line 2506: ThreadRestrictions::SetWaitAllowed(true); Can you restore this when you're done sleeping? I guess you'd need to capture the old value though. Line 2507: SleepFor(MonoDelta::FromMilliseconds(1024)); Or just sleep for 1s? PS1, Line 2512: mutable Not necessary; not using lock_ in a const method. Line 2513: bool is_first_call_; Could use an atomic bool and remove the lock altogether. PS1, Line 2516: const ::testing::TestInfo* const test_info = : ::testing::UnitTest::GetInstance()->current_test_info(); : const string kTestName = test_info->name(); Is this strictly necessary? Can't we just use a table name prefix like "foo"? PS1, Line 2530: replicated Why is replication important? If you had two tablets on just one server, wouldn't the client still send two Write RPCs? Line 2535: CreateTable(table_name, 3, std::move(splits), {}, &table); Should ASSERT_OK() on this. PS1, Line 2541: ASSERT_NO_FATAL_FAILURE Use NO_FATALS() instead. Line 2543: const Status s = session->Flush(); Could just ASSERT_OK() on this instead of saving 's'. Line 2551: ASSERT_OK(client_->DeleteTable(table_name)); Do you strictly need this? We'll delete all the tables when we destroy the cluster. -- To view, visit http://gerrit.cloudera.org:8080/5048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
