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/21 18:42:36 UTC
[kudu-CR] [scan] test for reusing snapshot timestamp when not set
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5163
Change subject: [scan] test for reusing snapshot timestamp when not set
......................................................................
[scan] test for reusing snapshot timestamp when not set
Added a test for reusing snapshot timestamp when not set while running
scans in READ_AT_SNAPSHOT mode. This is in the context of KUDU-1189.
The test would fail because the fix for KUDU-1189 is absent, that's why
it's disabled for a while. The fix is coming in one of the next changes,
which will enable the test.
Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
---
M src/kudu/client/client.h
M src/kudu/integration-tests/consistency-itest.cc
2 files changed, 211 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/5163/1
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] KUDU-1189 integration test for reusing snap timestamp
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
KUDU-1189 integration test for reusing snap timestamp
Added a test for reusing snapshot timestamp when not set while running
scans in READ_AT_SNAPSHOT mode. This is a test for the functionality
introduced in the context of KUDU-1189.
The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.
Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Reviewed-on: http://gerrit.cloudera.org:8080/5163
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client.h
M src/kudu/integration-tests/consistency-itest.cc
2 files changed, 201 insertions(+), 26 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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] KUDU-1189 integration test for reusing snap timestamp
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/5163
to look at the new patch set (#4).
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
KUDU-1189 integration test for reusing snap timestamp
Added a test for reusing snapshot timestamp when not set while running
scans in READ_AT_SNAPSHOT mode. This is a test for the functionality
introduced in the context of KUDU-1189.
The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.
Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
---
M src/kudu/client/client.h
M src/kudu/integration-tests/consistency-itest.cc
2 files changed, 201 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/5163/4
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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] KUDU-1189 integration test for reusing snap timestamp
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/5163
to look at the new patch set (#3).
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
KUDU-1189 integration test for reusing snap timestamp
Added a test for reusing snapshot timestamp when not set while running
scans in READ_AT_SNAPSHOT mode. This is a test for the functionality
introduced in the context of KUDU-1189.
The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.
Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
---
M src/kudu/client/client.h
M src/kudu/integration-tests/consistency-itest.cc
2 files changed, 202 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/5163/3
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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] [scan] test for reusing snapshot timestamp when not set
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [scan] test for reusing snapshot timestamp when not set
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5163/2//COMMIT_MSG
Commit Message:
PS2, Line 7: [scan] test for reusing snapshot timestamp when not set
maybe mention the jira. something like: KUDU-XXXX Integration test for timestamp reuse on snapshot scans with no timestamp set.
PS2, Line 12: The test would fail because the fix for KUDU-1189 is absent, that's why
: it's disabled for a while. The fix is coming in one of the next changes,
: which will enable the test.
How about: The test is disabled as it currently fails. A follow up patch will fix the bug and enable the test.
http://gerrit.cloudera.org:8080/#/c/5163/2/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS2, Line 226: UpdateClockForTabletAtKey
How about: UpdateClockForTabletHostingKey
PS2, Line 297:
missing a word? "about"?
PS2, Line 435: shared_ptr<KuduClient> client;
: ASSERT_OK(cluster_->CreateClient(nullptr, &client));
: // Scan the table at a snapshot: let the servers pick the timestamp.
: shared_ptr<KuduTable> table;
: ASSERT_OK(client->OpenTable(table_name_, &table));
nit add a CreateClientAndOpenTable helper?
PS2, Line 490: // Insert an additional row into the first tablet.
: ASSERT_OK(InsertTestRows(client.get(), table.get(), 1, 1));
what is the point of the additional write?
PS2, Line 521: const uint64_t ts_scan = cfg.snapshot_timestamp();
: const uint64_t ts_lo = client->GetLatestObservedTimestamp();
: const MonoDelta t_diff = t_after_scan - t_before_scan;
: // An additional check that relies on the assumption that
: // the difference between servers' clocks is much bigger compared
: // with the scan time. If the assumption does not hold, this check
: // does not prove anything, but it does not fail neither.
: ASSERT_GE(t_diff.ToMicroseconds(), ts_lo - ts_scan);
sorry I missed this before, but isn't this comparing a hybrid time difference with a physical time difference? That doesn't sound right and tbh I don't think this check is adding much to the test. Same goes for the previous block.
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: 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] [scan] test for reusing snapshot timestamp when not set
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [scan] test for reusing snapshot timestamp when not set
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5163/2/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS2, Line 435: shared_ptr<KuduClient> client;
: ASSERT_OK(cluster_->CreateClient(nullptr, &client));
: // Scan the table at a snapshot: let the servers pick the timestamp.
: shared_ptr<KuduTable> table;
: ASSERT_OK(client->OpenTable(table_name_, &table));
> That would decrease the boilerplate code in this test a bit (one less line
ah missed you were using the client below (thought for a sec it was only the scanner). makes sense to leave it then
PS2, Line 490: // Insert an additional row into the first tablet.
: ASSERT_OK(InsertTestRows(client.get(), table.get(), 1, 1));
> This is to check that the timestamp is taken from the first tablet server:
ah, makes sense. sgtm
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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-1189 integration test for reusing snap timestamp
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5163/3/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS3, Line 232: / Advance first partition's tablet server clock
> nit: this advances the clock of any partition, right?
Good catch. Yes, that's a copy-paste artifact.
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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-1189 integration test for reusing snap timestamp
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
Patch Set 4:
Does this need to be rebased on top of the patch that unbroke the build?
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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: No
[kudu-CR] [scan] test for reusing snapshot timestamp when not set
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/5163
to look at the new patch set (#2).
Change subject: [scan] test for reusing snapshot timestamp when not set
......................................................................
[scan] test for reusing snapshot timestamp when not set
Added a test for reusing snapshot timestamp when not set while running
scans in READ_AT_SNAPSHOT mode. This is in the context of KUDU-1189.
The test would fail because the fix for KUDU-1189 is absent, that's why
it's disabled for a while. The fix is coming in one of the next changes,
which will enable the test.
Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
---
M src/kudu/client/client.h
M src/kudu/integration-tests/consistency-itest.cc
2 files changed, 211 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/5163/2
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1189 integration test for reusing snap timestamp
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5163/3/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS3, Line 232: / Advance first partition's tablet server clock
nit: this advances the clock of any partition, right?
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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-1189 integration test for reusing snap timestamp
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1189 integration test for reusing snap timestamp
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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: No
[kudu-CR] [scan] test for reusing snapshot timestamp when not set
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [scan] test for reusing snapshot timestamp when not set
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5163/2//COMMIT_MSG
Commit Message:
PS2, Line 7: [scan] test for reusing snapshot timestamp when not set
> maybe mention the jira. something like: KUDU-XXXX Integration test for time
Done
PS2, Line 12: The test would fail because the fix for KUDU-1189 is absent, that's why
: it's disabled for a while. The fix is coming in one of the next changes,
: which will enable the test.
> How about: The test is disabled as it currently fails. A follow up patch wi
Yep, that sounds better -- it's terse and explains the essence.
http://gerrit.cloudera.org:8080/#/c/5163/2/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS2, Line 226: UpdateClockForTabletAtKey
> How about: UpdateClockForTabletHostingKey
ok, sure. I just tried to keep it shorter :)
PS2, Line 297:
> missing a word? "about"?
Done
PS2, Line 435: shared_ptr<KuduClient> client;
: ASSERT_OK(cluster_->CreateClient(nullptr, &client));
: // Scan the table at a snapshot: let the servers pick the timestamp.
: shared_ptr<KuduTable> table;
: ASSERT_OK(client->OpenTable(table_name_, &table));
> nit add a CreateClientAndOpenTable helper?
That would decrease the boilerplate code in this test a bit (one less line for those blocks), but I don't think it brings in any value from the semantic point of view. I.e., instead of
shared_ptr<KuduClient> client;
ASSERT_OK(cluster_->CreateClient(nullptr, &client));
shared_ptr<KuduTable> table;
ASSERT_OK(client->OpenTable(table_name_, &table));
there would be
shared_ptr<KuduClient> client;
shared_ptr<KuduTable> table;
ASSERT_OK(OpenTableWithNewClient(&client, &table));
That's said, I would leave this as is.
However, if you feel strongly about that, I'll change it.
PS2, Line 490: // Insert an additional row into the first tablet.
: ASSERT_OK(InsertTestRows(client.get(), table.get(), 1, 1));
> what is the point of the additional write?
This is to check that the timestamp is taken from the first tablet server: since now the clocks of both tablet servers are ahead of the timestamps of the inserted rows so far, there would be no way to tell which server's clock is used for the scan: in either case, there will be two result rows.
Now, once we add a new row into the first tablet, given the big time margin provided by the current clock offset, we should see different outcomes from the subsequent scan:
* if the timestamp is taken from the first server, there should be three rows in the result
* if the timestamp is taken from the second server, there should be just two rows in the result.
I'll add the corresponding comment into the code.
PS2, Line 521: const uint64_t ts_scan = cfg.snapshot_timestamp();
: const uint64_t ts_lo = client->GetLatestObservedTimestamp();
: const MonoDelta t_diff = t_after_scan - t_before_scan;
: // An additional check that relies on the assumption that
: // the difference between servers' clocks is much bigger compared
: // with the scan time. If the assumption does not hold, this check
: // does not prove anything, but it does not fail neither.
: ASSERT_GE(t_diff.ToMicroseconds(), ts_lo - ts_scan);
> sorry I missed this before, but isn't this comparing a hybrid time differen
ok, good point. Somehow I overlooked the fact the hybrid timestamp oranges are not those wall clock apples :)
Thank you for pointing at that -- will fix.
--
To view, visit http://gerrit.cloudera.org:8080/5163
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
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