[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Alexey Serbin (Code Review)]

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 

#incldue 
#include 


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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves