You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2016/11/20 02:33:44 UTC
[kudu-CR] Check that the set raw snapshot timestamp is > 0
David Ribeiro Alves has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5155
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Check that the set raw snapshot timestamp is > 0
Commit 06bb52d changed the default (invalid) timestamp to 0
which was previously a valid raw snapshot timestamp to set.
This adds a check to make sure we now return Status::IllegalState
in this case.
Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
---
M src/kudu/client/client.cc
1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5155/1
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Check that the set raw snapshot timestamp is > 0
Commit 06bb52d changed the default (invalid) timestamp to 0
which was previously a valid raw snapshot timestamp to set.
This adds a check to make sure we now return Status::IllegalState
in this case.
Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Reviewed-on: http://gerrit.cloudera.org:8080/5155
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/client/client.cc
1 file changed, 3 insertions(+), 0 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Patch Set 1:
Do we need an equivalent change on the Java side?
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Patch Set 2:
(1 comment)
> fuzz-itest used to create snapshots at timestamp 0, which
> previously were silently transformed to no "no timestamp" snapshot
> scans. This check avoids the silent transformation.
> looking back we probably shouldn't have changed the int64's to
> uint64's.
> On the server the "no timestamp" value is not 0 and we never do a
> max(no_timestamp, some_timestamp) like we do on the client.
I think the main issue here not about changing types, but having some special values. If it make sense, we could leave 0 as a valid timestamp, but instead add a boolean field in Scan::Configuration to determine a condition when timestamp is not set.
http://gerrit.cloudera.org:8080/#/c/5155/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:
PS2, Line 1092: IllegalState
I'm late here, but just to clarify: why IllegalState() but not InvalidArgument()?
Also, why not do add this check in Configuration::SetSnapshotRaw() method instead?
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Patch Set 1:
fuzz-itest used to create snapshots at timestamp 0, which previously were silently transformed to no "no timestamp" snapshot scans. This check avoids the silent transformation.
looking back we probably shouldn't have changed the int64's to uint64's.
On the server the "no timestamp" value is not 0 and we never do a max(no_timestamp, some_timestamp) like we do on the client.
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Patch Set 1:
not sure, nothing changed in the java client, but having the c++ not allow scans at 0 and allowing them in java seems silly. wdyt?
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Check that the set raw snapshot timestamp is > 0
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Check that the set raw snapshot timestamp is > 0
......................................................................
Patch Set 1:
what would happen server-side without this check? should the check be done server-side? curious what the motivation was for this
--
To view, visit http://gerrit.cloudera.org:8080/5155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No