You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2018/06/14 22:37:57 UTC
[kudu-CR] java client: Add accessor from sync scanner to async scanner
Hello Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10723
to review the following change.
Change subject: java client: Add accessor from sync scanner to async scanner
......................................................................
java client: Add accessor from sync scanner to async scanner
The async scanner underlies the sync scanner. Add an accessor from the
sync scanner so that the properties of the underlying scanner can be
easily inspected with only a reference to the sync scanner.
Change-Id: Ib459b399a6cfd4e9c23e4433dfcbe69e25e3b21d
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/10723/1
--
To view, visit http://gerrit.cloudera.org:8080/10723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib459b399a6cfd4e9c23e4433dfcbe69e25e3b21d
Gerrit-Change-Number: 10723
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] java client: Add accessor from sync scanner to async scanner
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10723 )
Change subject: java client: Add accessor from sync scanner to async scanner
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10723/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10723/1//COMMIT_MSG@11
PS1, Line 11: easily inspected with only a reference to the sync scanner.
> How about exposing the properties on the sync scanner? The async scanner's
Do you think someday we will break the async scanner API? I have a hard time imagining that. It seems error prone to duplicate the getters in the sync client as well, i.e. we might forget some. These are kind of orthogonal questions because maybe we should expose both the getters and the async scanner.
--
To view, visit http://gerrit.cloudera.org:8080/10723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib459b399a6cfd4e9c23e4433dfcbe69e25e3b21d
Gerrit-Change-Number: 10723
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 15 Jun 2018 16:39:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] java client: Add accessor from sync scanner to async scanner
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has abandoned this change. ( http://gerrit.cloudera.org:8080/10723 )
Change subject: java client: Add accessor from sync scanner to async scanner
......................................................................
Abandoned
I'm not really convinced we can break this API, we'll likely have to bridge it for many releases if we want to improve it later. But it's easy to punt on this for now and expose the getter I need in the sync scanner so I did that instead. :)
--
To view, visit http://gerrit.cloudera.org:8080/10723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ib459b399a6cfd4e9c23e4433dfcbe69e25e3b21d
Gerrit-Change-Number: 10723
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] java client: Add accessor from sync scanner to async scanner
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10723 )
Change subject: java client: Add accessor from sync scanner to async scanner
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10723/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10723/1//COMMIT_MSG@11
PS1, Line 11: easily inspected with only a reference to the sync scanner.
How about exposing the properties on the sync scanner? The async scanner's API isn't as stable as the sync scanner's.
http://gerrit.cloudera.org:8080/#/c/10723/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java:
http://gerrit.cloudera.org:8080/#/c/10723/1/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java@40
PS1, Line 40: * Returns the underlying AsyncKuduScanner instance.
Add @return statement.
--
To view, visit http://gerrit.cloudera.org:8080/10723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib459b399a6cfd4e9c23e4433dfcbe69e25e3b21d
Gerrit-Change-Number: 10723
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 15 Jun 2018 00:22:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] java client: Add accessor from sync scanner to async scanner
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10723 )
Change subject: java client: Add accessor from sync scanner to async scanner
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10723/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10723/1//COMMIT_MSG@11
PS1, Line 11: easily inspected with only a reference to the sync scanner.
> Do you think someday we will break the async scanner API?
I hope so. The deferred class the API is based on is terrible.
> It seems error prone to duplicate the getters in the sync client as well, i.e. we might forget some.
For the scanner builders we solved this by creating an abstract superclass, but I think that's a little bit too much overhead for the relatively small number of getters we need to expose. Doesn't seem like such a big issue to just do them on an as-needed basis.
> These are kind of orthogonal questions because maybe we should expose both the getters and the async scanner.
But in which situation would you want to switch from receiving synchronous scan results to async scan results with the same scanner instance? I can't think of any.
--
To view, visit http://gerrit.cloudera.org:8080/10723
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib459b399a6cfd4e9c23e4433dfcbe69e25e3b21d
Gerrit-Change-Number: 10723
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Mon, 18 Jun 2018 22:47:09 +0000
Gerrit-HasComments: Yes