Jean-Daniel Cryans has posted comments on this change. Change subject: [java-client] implement scan token API ......................................................................
Patch Set 2: (6 comments) A few initial comments. We also talked in person about how the configuration should be done for the MR job, we were previously thinking of having a "scan descriptor" object and it's not present in this patch. Then there's how things would be done in Spark. Let's take this to the dev@ list. http://gerrit.cloudera.org:8080/#/c/2592/2//COMMIT_MSG Commit Message: Line 10: toke nit http://gerrit.cloudera.org:8080/#/c/2592/2/java/kudu-client/src/main/java/org/kududb/client/Bytes.java File java/kudu-client/src/main/java/org/kududb/client/Bytes.java: Line 1105: b.array() Are you using the array here just to save a copy? A bulk get would do the job, and this isn't really on any performance critical path. http://gerrit.cloudera.org:8080/#/c/2592/2/java/kudu-client/src/main/java/org/kududb/client/KuduClient.java File java/kudu-client/src/main/java/org/kududb/client/KuduClient.java: Line 211: * Scan tokens may be used for creating parallel scanners over a Kudu table. If I was a random user reading this, I'd think it's pretty cool that I can create parallel scanners but I would have no idea how to do it. We should either make this more descriptive, or simply say that this is used by query engines and give an example. http://gerrit.cloudera.org:8080/#/c/2592/2/java/kudu-client/src/main/java/org/kududb/client/ScanToken.java File java/kudu-client/src/main/java/org/kududb/client/ScanToken.java: Line 36: public class ScanToken implements Comparable<ScanToken> { Add InterfaceAudience and InterfaceStability, plus javadoc (here and on the other public methods). Line 48: * Returns the tablet which this scan token will scan. I don't think it's the token that's scanning. http://gerrit.cloudera.org:8080/#/c/2592/2/src/kudu/client/CMakeLists.txt File src/kudu/client/CMakeLists.txt: Line 18: PROTOBUF_GENERATE_CPP( Feels like this file and the next should be in their own commit. -- To view, visit http://gerrit.cloudera.org:8080/2592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I20eff9bf51e893226fc3bc47726565ca62c054e3 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
