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/09/30 23:07:44 UTC
[kudu-CR] [client-test] added basic test for RWYW behavior
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4583
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
[client-test] added basic test for RWYW behavior
Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
---
M src/kudu/client/client-test.cc
1 file changed, 68 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/4583/1
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 1:
> I have an ambivalent opinion regarding this test.
>
> Read Your Writes means that a client can _always_ observe the write
> that was performed. If that is not the case then it's not RYW. We
> currently have gaps in our consistency story that we're looking to
> solve, so we don't get RYW in rare cases. To get around this the
> test loops until RYW occurs, which goes against the nature of what
> its testing. After all even the least consistent of eventual
> consistent databases would likely pass a test like this, given
> enough retries.
The test loops when it sees timeout from a tablet replica catching-up, not just to get expected number or rows.
>
> This is not to say that this test is without merit, IMO we could go
> about it two different ways:
>
> - Add this without the loops as a DISABLED_ test
> - Keep this patch open and re-evaluate merging later.
>
> That being said while fixing our consistency story we'll have to
> come with much more adversarial tests (with node kills, stops,
> elections, etc).
Yes, this is a very basic test to explicitly exercise 'timestamp + 1' notion. It does not pretend to be an exhaustive one -- that one should be among integration tests, not here.
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 2:
As I had suggested if you made so that the test always failed, we could merge it disabled and then re-enable it when we get this stuff fixed.
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
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/4583
to look at the new patch set (#2).
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
[client-test] added basic test for RWYW behavior
Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
---
M src/kudu/client/client-test.cc
1 file changed, 69 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/4583/2
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 1:
I have an ambivalent opinion regarding this test.
Read Your Writes means that a client can _always_ observe the write that was performed. If that is not the case then it's not RYW. We currently have gaps in our consistency story that we're looking to solve, so we don't get RYW in rare cases. To get around this the test loops until RYW occurs, which goes against the nature of what its testing. After all even the least consistent of eventual consistent databases would likely pass a test like this, given enough retries.
This is not to say that this test is without merit, IMO we could go about it two different ways:
- Add this without the loops as a DISABLED_ test
- Keep this patch open and re-evaluate merging later.
That being said while fixing our consistency story we'll have to come with much more adversarial tests (with node kills, stops, elections, etc).
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Abandoned
This test's intent was to explicitly document (timestamp + 1) notion while using corresponding API methods to achive RWYW behavior. That was addressed by the updated in-source doxygen documentation.
As for providing decent coverage of RWYW behavior, more robust test is need because there are many corner cases there. Probably, it should be a new integration test. David Alves has more context on that.
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 1:
> |The test loops when it sees timeout from a tablet replica
> catching-up, not just to get expected number or rows.
>
> You're right, but it also does loop for RYW meaning it's not
> actually testing RYW. Fwiw I like the documentation value of the
> test (i.e. it's documenting the _hack_ we need to currently do to
> _likely_ get RYW) just not sure what it's currently testing. Maybe
> we could add this to the docs or something?
Sure, that's an options as well. Since you know best on this matter, I think we should do whatever you think is the better option.
OK, let me then update this changeset just to add some comments in the in-line documentation for both C++ and Java clients.
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 1:
Btw, a more interesting test, perhaps, would be one that _always_ failed, i.e. that made sure to cover the gap where RYW fails. That one would be good to have in the code based, DISABLED_
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 1:
> Btw, a more interesting test, perhaps, would be one that _always_
> failed, i.e. that made sure to cover the gap where RYW fails. That
> one would be good to have in the code based, DISABLED_
OK, I see.
We can abandon this test and create a new one using the scenario you described (probably, it should be among the integration tests).
Could you elaborate a little bit more on the scenario to for the test you are advocating? I'd like to use that as a guide and description for that test. Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 1:
|The test loops when it sees timeout from a tablet replica catching-up, not just to get expected number or rows.
You're right, but it also does loop for RYW meaning it's not actually testing RYW. Fwiw I like the documentation value of the test (i.e. it's documenting the _hack_ we need to currently do to _likely_ get RYW) just not sure what it's currently testing. Maybe we could add this to the docs or something?
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No
[kudu-CR] [client-test] added basic test for RWYW behavior
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] added basic test for RWYW behavior
......................................................................
Patch Set 2:
(1 comment)
> As I had suggested if you made so that the test always failed, we
> could merge it disabled and then re-enable it when we get this
> stuff fixed.
I think we can abandon this change if it does not provide good guarantees on exercising RYW behavior. Let's figure out what behavior we want from more elaborate test and put that into the pool of integration tests.
http://gerrit.cloudera.org:8080/#/c/4583/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
The patchset 2 is just to provide an example for the case I mentioned in the comment for item '4. Potential unrepeatable read' at https://docs.google.com/document/d/1EaKlJyQdMBz6G-Xn5uktY-d_x0uRmjMCrDGP5rZ7AoI
--
To view, visit http://gerrit.cloudera.org:8080/4583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3c237a41a1434199848ac5fc978fab27e565e2d8
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes