You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/11/29 13:54:28 UTC
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Hello David Ribeiro Alves, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5256
to review the following change.
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
WIP: KUDU-1767. Create a client flush interleave test
This test shows that it is possible to get out-of-order writes when
writing from a single KuduSession.
This test currently fails.
Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/client_flush_interleave-itest.cc
2 files changed, 129 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5256/1
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5256
to look at the new patch set (#2).
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
WIP: KUDU-1767. Create a client flush interleave test
This test shows that it is possible to get out-of-order writes when
writing from a single KuduSession.
This test currently fails.
Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/client_flush_interleave-itest.cc
2 files changed, 111 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5256/2
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5256/2/src/kudu/integration-tests/client_flush_interleave-itest.cc
File src/kudu/integration-tests/client_flush_interleave-itest.cc:
Line 17:
> nit: consider adding
Included the first two, but gtest is superfluous when inheriting from MiniClusterITestBase.
PS2, Line 44: / Check that attempts to scan prior to the ancient history mark fail
> The comment looks outdated.
Done
PS2, Line 71: KuduUpdate* update
> nit: consider using unique_ptr to wrap the update op in case of any of the
Done
PS2, Line 76: for (int i = 0; i < kMax; i++) {
: ASSERT_OK(session->Apply(updates[i]));
: session->FlushAsync(nullptr);
: }
> if using unique_ptr, probably you could use deque instead of vector or inse
Do you have a specific concern? deques tend to be slower than vectors / arrays because the list links defeat memory locality / prefetching. Using a vector in this manner seems reasonable to me both from a performance and a readability standpoint. How about I just reserve() the capacity in advance?
PS2, Line 91: LOG(INFO)
> Does it make sense to use VLOG() here instead? Or it's crucial to have tho
Done
PS2, Line 97: "int_val=$0"
> What do those lines look like? My concern is that some portion of anomalie
Added a ")" which should do it.
PS2, Line 101: /*
: KuduScanner scanner(table.get());
: ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
: Status s = scanner.Open();
: ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
: ASSERT_STR_CONTAINS(s.ToString(), "Snapshot timestamp is earlier than the ancient history mark");
: */
> nit: consider removing this commented code.
Done
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
Patch Set 2:
(7 comments)
It looks like a nice test to isolate this problem!
http://gerrit.cloudera.org:8080/#/c/5256/2/src/kudu/integration-tests/client_flush_interleave-itest.cc
File src/kudu/integration-tests/client_flush_interleave-itest.cc:
Line 17:
nit: consider adding
#include <vector>
#incldue <glog/logging.h>
#include <gtest/gtest.h>
PS2, Line 44: / Check that attempts to scan prior to the ancient history mark fail
The comment looks outdated.
PS2, Line 71: KuduUpdate* update
nit: consider using unique_ptr to wrap the update op in case of any of the ASSERT_OK() below fail (I suspect that could lead to ASAN warnings)
PS2, Line 76: for (int i = 0; i < kMax; i++) {
: ASSERT_OK(session->Apply(updates[i]));
: session->FlushAsync(nullptr);
: }
if using unique_ptr, probably you could use deque instead of vector or inserts those updates in reverse order and then do pop_back() if using the vector container.
PS2, Line 91: LOG(INFO)
Does it make sense to use VLOG() here instead? Or it's crucial to have those lines always printed out?
PS2, Line 97: "int_val=$0"
What do those lines look like? My concern is that some portion of anomalies could be missed here; e.g., consider
'int_val=1' and 'int_val=10' and alike.
May be, it's worth to update the check to make sure those cases are also covered.
PS2, Line 101: /*
: KuduScanner scanner(table.get());
: ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
: Status s = scanner.Open();
: ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
: ASSERT_STR_CONTAINS(s.ToString(), "Snapshot timestamp is earlier than the ancient history mark");
: */
nit: consider removing this commented code.
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1767. Create a client flush interleave test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5256
to look at the new patch set (#3).
Change subject: KUDU-1767. Create a client flush interleave test
......................................................................
KUDU-1767. Create a client flush interleave test
This test shows that it is possible to get out-of-order writes when
writing from a single KuduSession.
This test currently fails, so it is disabled.
Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/client_flush_interleave-itest.cc
2 files changed, 133 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5256/3
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
Patch Set 2:
Thanks for the review, Alexey. Yes, this test is currently pretty messy. I do need to clean it up but I think we need to come up with a real solution to the problem it exposes before I need to do that. :)
Based on comments on KUDU-1767 I think we'll be prioritizing some other things sooner.
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] KUDU-1767. Create a client flush interleave test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change.
Change subject: KUDU-1767. Create a client flush interleave test
......................................................................
Abandoned
We decided that, at least for now, this will not be fixed. See KUDU-1767.
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
Patch Set 2:
> ok, I see.
I think you assumed we would check this in DISABLED. It's actually something I hadn't considered before I saw you doing that. I guess it's not a bad idea...
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP: KUDU-1767. Create a client flush interleave test
......................................................................
Patch Set 2:
> Thanks for the review, Alexey. Yes, this test is currently pretty
> messy. I do need to clean it up but I think we need to come up with
> a real solution to the problem it exposes before I need to do that.
> :)
>
> Based on comments on KUDU-1767 I think we'll be prioritizing some
> other things sooner.
ok, I see.
--
To view, visit http://gerrit.cloudera.org:8080/5256
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No