Alexey Serbin has posted comments on this change. Change subject: [c++client] fixed KUDU-1743 ......................................................................
Patch Set 1: (15 comments) http://gerrit.cloudera.org:8080/#/c/5048/1/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: Line 726: // TODO: handle case where we get one of the more specific TS errors > warning: missing username/bug in TODO [google-readability-todo] Done 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 Done 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. Done 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 Done Line 2488: class CustomErrorCollector : public ErrorCollector { > Another option is to use MAYBE_INJECT_RANDOM_LATENCY() in the regular error I would opt for this custom collector: I don't like the idea adding too much of test-related stuff into the production code, otherwise it's scary to read that. Line 2506: ThreadRestrictions::SetWaitAllowed(true); > Can you restore this when you're done sleeping? I guess you'd need to captu Done Line 2507: SleepFor(MonoDelta::FromMilliseconds(1024)); > Or just sleep for 1s? Done PS1, Line 2512: mutable > Not necessary; not using lock_ in a const method. Done Line 2513: bool is_first_call_; > Could use an atomic bool and remove the lock altogether. Good idea: I thought about that, but then just copy-pasted the mutex solution from somewhere hastily. Will try atomics. 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 I thought this might be useful if we don't want to care about uniqueness of those prefixes. But if you insist, I'll will do it, sure. PS1, Line 2530: replicated > Why is replication important? If you had two tablets on just one server, wo For some reason, those responses in non-replicated case come from the same reactor thread, sequentially. I haven't found a better way to mitigate that. If you have a better idea how to make it being called from different threads, I'll gladly remove the replication. Line 2535: CreateTable(table_name, 3, std::move(splits), {}, &table); > Should ASSERT_OK() on this. Done PS1, Line 2541: ASSERT_NO_FATAL_FAILURE > Use NO_FATALS() instead. Done Line 2543: const Status s = session->Flush(); > Could just ASSERT_OK() on this instead of saving 's'. Done Line 2551: ASSERT_OK(client_->DeleteTable(table_name)); > Do you strictly need this? We'll delete all the tables when we destroy the After many runs (I tried larger number of iterations) it left too little space on my drive. But you are right -- now it's not that many, so will drop this. -- 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 <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
