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/24 22:48:20 UTC
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5220
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
[c++ client] timestamp propagation via scan tokens
Implemented server timestamp propagation via scan tokens for the
Kudu C++ client library. Added corresponding unit test as well.
Besides, this changelist also enables the
ConsistencyITest.TestScanTokenTimestampPropagation test: this patch
brings the necessary fix to make it pass.
This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens
Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
---
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/consistency-itest.cc
5 files changed, 88 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5220/1
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 1:
did you test that these tokens are cross-client compatible?
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5220/1/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 328: // latest observed timestamp of the client object
> missing period.
Done
Line 330: // When creating serialized scan tokens,
> redundant paragraph?
Done
Line 366: const uint64_t ts_prev = client_->GetLatestObservedTimestamp();
> Is this going to 'break' the client for subsequent tests?
Do you mean that advancing the latest observed timestamp in the client could break the client?
If I'm not mistaken, the client is re-created for every test due to the the SetUp()/WrapUp() sequence in the base test, so it should not be a problem.
Or your questions is about something else?
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-420 [c++ client] timestamp propagation via scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5220
to look at the new patch set (#4).
Change subject: KUDU-420 [c++ client] timestamp propagation via scan tokens
......................................................................
KUDU-420 [c++ client] timestamp propagation via scan tokens
Implemented server timestamp propagation via scan tokens for the
Kudu C++ client library. Added corresponding unit test as well.
Besides, this changelist also enables the
ConsistencyITest.TestScanTokenTimestampPropagation test: this patch
brings the necessary fix to make it pass.
This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens
Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
---
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/consistency-itest.cc
5 files changed, 87 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5220/4
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5220/1/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 366: const uint64_t ts_prev = client_->GetLatestObservedTimestamp();
> Do you mean that advancing the latest observed timestamp in the client coul
Nope, that answers my question. I misremembered and thought the client wasn't recreated per test.
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5220/1/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
Line 328: // latest observed timestamp of the client object
missing period.
Line 330: // When creating serialized scan tokens,
redundant paragraph?
Line 366: const uint64_t ts_prev = client_->GetLatestObservedTimestamp();
Is this going to 'break' the client for subsequent tests?
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-420 [c++ client] timestamp propagation via scan tokens
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-420 [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 2: Code-Review+2
Would like your thoughts on my q, but I don't think that should be an impediment to the patch.
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 2:
> Would like your thoughts on my q, but I don't think that should be
> an impediment to the patch.
I see, thank you for the clarification.
As for you question, from what I see, I would not expect the thing to break due to that patch if the inter-operability was OK prior to that. Consider two things:
* The raw format compatibility is guaranteed by the ProtoBuf
* As for the application logic of serializing/de-serializing, the patch does not add any new requirements/expectations on the data contained in the serialized token.
I opened a JIRA item on implementing a test for the inter-operability of those tokens:
https://issues.apache.org/jira/browse/KUDU-1765
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] KUDU-420 [c++ client] timestamp propagation via scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: KUDU-420 [c++ client] timestamp propagation via scan tokens
......................................................................
KUDU-420 [c++ client] timestamp propagation via scan tokens
Implemented server timestamp propagation via scan tokens for the
Kudu C++ client library. Added corresponding unit test as well.
Besides, this changelist also enables the
ConsistencyITest.TestScanTokenTimestampPropagation test: this patch
brings the necessary fix to make it pass.
This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens
Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Reviewed-on: http://gerrit.cloudera.org:8080/5220
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/consistency-itest.cc
5 files changed, 87 insertions(+), 9 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5220
to look at the new patch set (#3).
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
[c++ client] timestamp propagation via scan tokens
Implemented server timestamp propagation via scan tokens for the
Kudu C++ client library. Added corresponding unit test as well.
Besides, this changelist also enables the
ConsistencyITest.TestScanTokenTimestampPropagation test: this patch
brings the necessary fix to make it pass.
This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens
Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
---
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/consistency-itest.cc
5 files changed, 87 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5220/3
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [c++ client] timestamp propagation via scan tokens
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/5220
to look at the new patch set (#2).
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
[c++ client] timestamp propagation via scan tokens
Implemented server timestamp propagation via scan tokens for the
Kudu C++ client library. Added corresponding unit test as well.
Besides, this changelist also enables the
ConsistencyITest.TestScanTokenTimestampPropagation test: this patch
brings the necessary fix to make it pass.
This is in the context of the following JIRA item:
KUDU-420 c++ client: implement HT timestamp propagation via scan tokens
Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
---
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/integration-tests/consistency-itest.cc
5 files changed, 87 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5220/2
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
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: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 1:
> did you test that these tokens are cross-client compatible?
Nope, that I didn't do. I assumed that's guaranteed by the ProtoBuffer format as is.
Or you mean some sort of artificial constraints in the serialization/de-serialization logic in Java and C++ client?
We could implement a test for the latter as well (if it does not exist). I would vote for doing that, but I think it's better to do that in a separate changelist.
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [c++ client] timestamp propagation via scan tokens
......................................................................
Patch Set 1:
I meant making sure that we're not breaking anything with this change stand-alone. We usually get the scan tokens from the java client and then use then in the c++ client. would that still work with this change?
--
To view, visit http://gerrit.cloudera.org:8080/5220
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No