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/11 03:22:19 UTC
[kudu-CR] [c++client] fixed KUDU-1743
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5048
Change subject: [c++client] fixed KUDU-1743
......................................................................
[c++client] fixed KUDU-1743
KUDU-1743: GetPendingErrors() not returning all errors after Flush()
Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.h
6 files changed, 106 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5048/1
--
To view, visit http://gerrit.cloudera.org:8080/5048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [c++client] fixed KUDU-1743
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/5048
to look at the new patch set (#2).
Change subject: [c++client] fixed KUDU-1743
......................................................................
[c++client] fixed KUDU-1743
KUDU-1743: GetPendingErrors() not returning all errors after Flush()
A race has been discovered during Impala + Kudu testing: if working
with tables split among multiple tablet servers, a rare race condition
manifests itself as not having all the errorrs in the error collector
upon return from the KuduSession::Flush() method.
This patch fixes the race and adds corresponding test for that.
Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.h
6 files changed, 121 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5048/2
--
To view, visit http://gerrit.cloudera.org:8080/5048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [c++client] fixed KUDU-1743
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [c++client] fixed KUDU-1743
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5048/3/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
PS3, Line 748: is
Nit: was
PS3, Line 754: CheckIfFinishedFlush
CheckForFinishedFlush()
Line 756: // * T2 does the same
Nit: could you terminate all the sentences here with periods?
http://gerrit.cloudera.org:8080/#/c/5048/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1, Line 2516: vector<unique_ptr<KuduPartialRow>> splits;
: unique_ptr<KuduPartialRow> split(schema_.NewRow());
: ASSERT_OK(split->SetInt32("key", kRowNum
> I thought this might be useful if we don't want to care about uniqueness of
Wouldn't the table names be unique even if they were just Substitute("$0", i) ? I guess I'm not understanding what you're concerned about.
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++client] fixed KUDU-1743
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++client] fixed KUDU-1743
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/5048
to look at the new patch set (#3).
Change subject: [c++client] fixed KUDU-1743
......................................................................
[c++client] fixed KUDU-1743
KUDU-1743: GetPendingErrors() not returning all errors after Flush()
A race has been discovered during Impala + Kudu testing: if working
with tables split among multiple tablet servers, a rare race condition
manifests itself as not having all the errorrs in the error collector
upon return from the KuduSession::Flush() method.
This patch fixes the race and adds corresponding test for that.
Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.h
6 files changed, 120 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5048/3
--
To view, visit http://gerrit.cloudera.org:8080/5048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [c++client] fixed KUDU-1743
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/5048
to look at the new patch set (#4).
Change subject: [c++client] fixed KUDU-1743
......................................................................
[c++client] fixed KUDU-1743
KUDU-1743: GetPendingErrors() not returning all errors after Flush()
A race has been discovered during Impala + Kudu testing: if working
with tables split among multiple tablet servers, a rare race condition
manifests itself as not having all the errorrs in the error collector
upon return from the KuduSession::Flush() method.
This patch fixes the race and adds corresponding test for that.
Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.h
6 files changed, 116 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5048/4
--
To view, visit http://gerrit.cloudera.org:8080/5048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [c++client] fixed KUDU-1743
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++client] fixed KUDU-1743
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++client] fixed KUDU-1743
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5048/3/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
PS3, Line 748: is
> Nit: was
Done
PS3, Line 754: CheckIfFinishedFlush
> CheckForFinishedFlush()
Done
Line 756: // * T2 does the same
> Nit: could you terminate all the sentences here with periods?
Done
http://gerrit.cloudera.org:8080/#/c/5048/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1, Line 2516: vector<unique_ptr<KuduPartialRow>> splits;
: unique_ptr<KuduPartialRow> split(schema_.NewRow());
: ASSERT_OK(split->SetInt32("key", kRowNum
> Wouldn't the table names be unique even if they were just Substitute("$0",
Ah, yes -- everything is teared down after a test finishes.
--
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [c++client] fixed KUDU-1743
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [c++client] fixed KUDU-1743
......................................................................
Patch Set 4: Code-Review+2
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [c++client] fixed KUDU-1743
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [c++client] fixed KUDU-1743
......................................................................
[c++client] fixed KUDU-1743
KUDU-1743: GetPendingErrors() not returning all errors after Flush()
A race has been discovered during Impala + Kudu testing: if working
with tables split among multiple tablet servers, a rare race condition
manifests itself as not having all the errorrs in the error collector
upon return from the KuduSession::Flush() method.
This patch fixes the race and adds corresponding test for that.
Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
Reviewed-on: http://gerrit.cloudera.org:8080/5048
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.h
M src/kudu/client/session-internal.h
6 files changed, 116 insertions(+), 20 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3054d7f7c00dd937f4307fb01a5a0054b8ae27f7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>