[kudu-CR] [i-tests] scan token timestamp propagation test

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] [i-tests] scan token timestamp propagation test

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/3//COMMIT_MSG
Commit Message:

Line 15: This is in the context of the following JIRA item:
I think it's better to simply put "KUDU-420. " at the beginning of the commit 
message.


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5219

to look at the new patch set (#3).

Change subject: [i-tests] scan token timestamp propagation test
..

[i-tests] scan token timestamp propagation test

Added an integration test to verify timestamp propagation via scan
tokens for C++ client.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens

Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
---
M src/kudu/integration-tests/consistency-itest.cc
1 file changed, 107 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5219/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 611: ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), 
scan_token,
 : 
_raw));
> It could be a good solution, however the scan below does not succeed, so th
ok, it seems I see what you mean: that's what  we should see in case of 
success.  If it fails, it fails and we don't do any conclusions -- that's the 
essence.

All right, this sounds good to me.  Will update in a moment.


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 611: ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), 
scan_token,
 : 
_raw));
> how about here we assert that the lot here is 'ts' from the block above and
It could be a good solution, however the scan below does not succeed, so this 
would not work.

Or some other changes are needed as well?


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 611: ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), 
scan_token,
 : 
_raw));
how about here we assert that the lot here is 'ts' from the block above and at 
that the lot after each scan is higher?


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

Line 591: ASSERT_OK(builder.SetSnapshotRaw(ts));
> maybe I'm missing something: if the scan tokens are propagating timestamps 
Yes, it seems I see your point.  Right, that's the essence of the timestamp 
propagation.

May be, I misunderstood Dan's comment -- I thought Dan asked whether the 
timestamp for the token-to-be is assigned implicitly from the client's latest 
observed one, i.e. whether the generated token would have the timestamp set 
even if we didn't set it explicitly to any value.  In my comment I addressed 
that thing.  That's because I saw some sort of parallel in here: check 
scanner-internal.cc, line 423. 

OK, it seems it's just my misunderstanding.  I'll remove the timestamp 
assignment above.


PS1, Line 606: shared_ptr client;
 : ASSERT_OK(cluster_->CreateClient(nullptr, ));
 : shared_ptr table;
 : ASSERT_OK(client->OpenTable(table_name_, ));
 : KuduScanner* scanner_raw;
 : 
ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token,
 : 
_raw));
 : unique_ptr scanner(scanner_raw);
 : scanner->SetTimeoutMillis(scan_timeout_msec);
 : ASSERT_OK(scanner->Open());
 : size_t row_num = 0;
 : while (scanner->HasMoreRows()) {
 :   KuduScanBatch batch;
 :   ASSERT_OK(scanner->NextBatch());
 :   row_num += batch.NumRows();
 : }
 : EXPECT_EQ(0, row_num);
> the trickyness stems from the fact that you could get timeouts that are unr
ok, I see; that makes sense.  I could not see other way of checking that.  Will 
need to explore other alternatives around.


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

Line 591: ASSERT_OK(builder.SetSnapshotRaw(ts));
> Currently that's not the case -- the ScanConfiguration object underneath th
maybe I'm missing something: if the scan tokens are propagating timestamps 
correctly and we don't set a timestamp on the snapshot scan then the server 
should choose a timestamp that is higher than the last write. that is the point 
of timestamp propagation in scan tokens no?


PS1, Line 606: shared_ptr client;
 : ASSERT_OK(cluster_->CreateClient(nullptr, ));
 : shared_ptr table;
 : ASSERT_OK(client->OpenTable(table_name_, ));
 : KuduScanner* scanner_raw;
 : 
ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token,
 : 
_raw));
 : unique_ptr scanner(scanner_raw);
 : scanner->SetTimeoutMillis(scan_timeout_msec);
 : ASSERT_OK(scanner->Open());
 : size_t row_num = 0;
 : while (scanner->HasMoreRows()) {
 :   KuduScanBatch batch;
 :   ASSERT_OK(scanner->NextBatch());
 :   row_num += batch.NumRows();
 : }
 : EXPECT_EQ(0, row_num);
> What's exactly tricky?  I don't see any trickiness here -- please check my 
the trickyness stems from the fact that you could get timeouts that are 
unrelated with what you are testing and that the test is only doing an indirect 
check. My comment was: can you do direct check somewhere? Its cool if not 
possible but if it is it would be preferable


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5219

to look at the new patch set (#2).

Change subject: [i-tests] scan token timestamp propagation test
..

[i-tests] scan token timestamp propagation test

Added an integration test to verify timestamp propagation via scan
tokens for C++ client.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens

Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
---
M src/kudu/integration-tests/consistency-itest.cc
1 file changed, 104 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5219/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 397: * Discard the client object.
> why is it relevant to discard the client at the end? are you doing anything
It's not that crucial, actually.  I just put it here to be in line with the 
rest of the list.

If it makes people wonder what it is, I will remove this step since it's not 
that crucial for understanding the idea behind the test.


PS1, Line 526: // The real-world use-case behind this test is as following:
 : //
 : //   * An external entity such as Spark job writes some data 
into the Kudu
 : // database. Then, it builds a scan token which would allow 
to fetch
 : // some related data at the specified timestamp. The scan 
token is
 : // serialized and output as a string.
 : //
 : //   * Another external entity (e.g., another client application 
based
 : // on the Kudu C++ client API) is fed with the serialized 
scan token
 : // obtained at the previous step. It performs a scan 
operation built
 : // from the scan token it's fed with.
> maybe be a bit more abstract? i mean no need to be talking specific scenari
Done


PS1, Line 540: , acting as an external entity,
> what does this mean?
Well, nothing particular -- all clients act as external entities.

The idea was to express that it's possible to pass scan tokens between 
unrelated client applications.

I'll remove this wording.


PS1, Line 541: which clock is advanced.
> missing some words/rephrase?
Done


PS1, Line 552: since the timeout for the scan operation
 : // is set to much shorter time interval
> aren't you using a mock clock that doesn't advance by itself? why does the 
I wanted to point that the test would not hang, but ran fast and reported an 
error right away due the the described facts.


PS1, Line 555: TEST_F(ConsistencyITest, 
DISABLED_TestScanTokenTimestampPropagation)
> this fails all the time without the fix, right?
Yes, it does fail without the fix, 100%


Line 591: ASSERT_OK(builder.SetSnapshotRaw(ts));
> Why do you have to set the raw snapshot timestamp?  Shouldn't it be propaga
Currently that's not the case -- the ScanConfiguration object underneath the 
KuduScanTokenBuilder is not implicitly affected while building the tokens.  
That sort of 'automation' is available only for KuduScanner while calling 
OpenTablet() method (i.e. while performing a scan operation at the table).

However, you make a good point here -- may be we should make 
KuduScanTokenBuilder and KuduScanner similar in that regard.

David, what do you think?


PS1, Line 606: shared_ptr client;
 : ASSERT_OK(cluster_->CreateClient(nullptr, ));
 : shared_ptr table;
 : ASSERT_OK(client->OpenTable(table_name_, ));
 : KuduScanner* scanner_raw;
 : 
ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token,
 : 
_raw));
 : unique_ptr scanner(scanner_raw);
 : scanner->SetTimeoutMillis(scan_timeout_msec);
 : ASSERT_OK(scanner->Open());
 : size_t row_num = 0;
 : while (scanner->HasMoreRows()) {
 :   KuduScanBatch batch;
 :   ASSERT_OK(scanner->NextBatch());
 :   row_num += batch.NumRows();
 : }
 : EXPECT_EQ(0, row_num);
> I think basing the test on a timeout is tricky, is there a way to make an a
What's exactly tricky?  I don't see any trickiness here -- please check my 
previous comment.


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 397: * Discard the client object.
why is it relevant to discard the client at the end? are you doing anything 
more after this? if so maybe say what it is?


PS1, Line 526: // The real-world use-case behind this test is as following:
 : //
 : //   * An external entity such as Spark job writes some data 
into the Kudu
 : // database. Then, it builds a scan token which would allow 
to fetch
 : // some related data at the specified timestamp. The scan 
token is
 : // serialized and output as a string.
 : //
 : //   * Another external entity (e.g., another client application 
based
 : // on the Kudu C++ client API) is fed with the serialized 
scan token
 : // obtained at the previous step. It performs a scan 
operation built
 : // from the scan token it's fed with.
maybe be a bit more abstract? i mean no need to be talking specific scenarios 
in the test IMO. just mention that we can get scan tokens from a client 
(c++/java) and use them on another client (c++/java) and that the lpt should be 
set properly.


PS1, Line 540: , acting as an external entity,
what does this mean?


PS1, Line 541: which clock is advanced.
missing some words/rephrase?


PS1, Line 552: since the timeout for the scan operation
 : // is set to much shorter time interval
aren't you using a mock clock that doesn't advance by itself? why does the 
timeout matter?


PS1, Line 555: TEST_F(ConsistencyITest, 
DISABLED_TestScanTokenTimestampPropagation)
this fails all the time without the fix, right?


PS1, Line 606: shared_ptr client;
 : ASSERT_OK(cluster_->CreateClient(nullptr, ));
 : shared_ptr table;
 : ASSERT_OK(client->OpenTable(table_name_, ));
 : KuduScanner* scanner_raw;
 : 
ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token,
 : 
_raw));
 : unique_ptr scanner(scanner_raw);
 : scanner->SetTimeoutMillis(scan_timeout_msec);
 : ASSERT_OK(scanner->Open());
 : size_t row_num = 0;
 : while (scanner->HasMoreRows()) {
 :   KuduScanBatch batch;
 :   ASSERT_OK(scanner->NextBatch());
 :   row_num += batch.NumRows();
 : }
 : EXPECT_EQ(0, row_num);
I think basing the test on a timeout is tricky, is there a way to make an 
assertion on actual clock setting/timestamp movement?


-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5219

Change subject: [i-tests] scan token timestamp propagation test
..

[i-tests] scan token timestamp propagation test

Added an integration test to verify timestamp propagation via scan
tokens for C++ client.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens

Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
---
M src/kudu/integration-tests/consistency-itest.cc
1 file changed, 111 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/5219/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5219
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin