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/28 20:57:34 UTC
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5248
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
[java] KUDU-1679 Propagate timestamps for scans
This is Java counterpart for 06bb52d2acc6d311144aa905101ec5d846096611.
Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
2 files changed, 19 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5248/1
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 2:
(2 comments)
lgtm just a couple of verbage nits. feel free to push after fixing.
http://gerrit.cloudera.org:8080/#/c/5248/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java:
PS2, Line 209: the propagated timestamp should be received.
nit avoid should, nit, rephrase to: "so the client should have received the propagated timestamp in the scanner response"
PS2, Line 217: bring
nit: s/bring/move s/the timestamp/the propagated timestamp
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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-HasComments: Yes
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 1:
(4 comments)
add a test?
http://gerrit.cloudera.org:8080/#/c/5248/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 203: This is only visible within the package.
see my comment below
PS1, Line 207: synchronized void updateLastPropagatedTimestamp(long lastPropagatedTimestamp)
see my comment below
PS1, Line 214: protected synchronized long getLastPropagatedTimestamp()
how are users supposed to get the lpt to pass it to other clients now if they are using the async client?
http://gerrit.cloudera.org:8080/#/c/5248/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:
PS1, Line 806: -1
use the constant?
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
Gerrit-PatchSet: 1
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-HasComments: Yes
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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-HasComments: No
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5248/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 214: protected synchronized long getLastPropagatedTimestamp()
> the whole point of the lpt is that users can use message passing to get lin
All right, that makes sense to me. Probably, the way we pass information about the last observed (propagated) timestamp would change, but at least we need to have some parity between the clients.
http://gerrit.cloudera.org:8080/#/c/5248/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:
PS1, Line 806: -1
> use the constant?
Done
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5248
to look at the new patch set (#3).
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
[java] KUDU-1679 Propagate timestamps for scans
This is Java counterpart for 06bb52d2acc6d311144aa905101ec5d846096611.
Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
3 files changed, 49 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5248/3
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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-HasComments: No
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
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/5248
to look at the new patch set (#2).
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
[java] KUDU-1679 Propagate timestamps for scans
This is Java counterpart for 06bb52d2acc6d311144aa905101ec5d846096611.
Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
3 files changed, 48 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/5248/2
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
[java] KUDU-1679 Propagate timestamps for scans
This is Java counterpart for 06bb52d2acc6d311144aa905101ec5d846096611.
Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
Reviewed-on: http://gerrit.cloudera.org:8080/5248
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
3 files changed, 49 insertions(+), 9 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5248/2/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java:
PS2, Line 209: the propagated timestamp should be received.
> nit avoid should, nit, rephrase to: "so the client should have received the
Done
PS2, Line 217: bring
> nit: s/bring/move s/the timestamp/the propagated timestamp
Done
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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-HasComments: Yes
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5248/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 214: protected synchronized long getLastPropagatedTimestamp()
> how are users supposed to get the lpt to pass it to other clients now if th
That's an interesting observation.
I thought it should reside within the client only. BTW, with the C++ client we have the same situation, right?
Do we want to expose it somehow?
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-1679 Propagate timestamps for scans
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.
Change subject: [java] KUDU-1679 Propagate timestamps for scans
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5248/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
PS1, Line 214: protected synchronized long getLastPropagatedTimestamp()
> That's an interesting observation.
the whole point of the lpt is that users can use message passing to get linearizabiilty so it should always be user visible. We're just making use of it ourselves for emulating a single client from many clients scenario. BTW Get/SetLatestObservedTimestamp are public methods on the c++ client
--
To view, visit http://gerrit.cloudera.org:8080/5248
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I84d45ba395f2a3fc6b54591f4a45bb4f10435910
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: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes