Mitch Barnett has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 )
Change subject: [examples] Add basic Spark example written in Scala ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/README.adoc@43 PS1, Line 43: To specify a value at execution time, you'll specify the parameter name in the '-D<name>' format. For example, to set a different set of masters for the Kudu cluster from the command line and use a custom table name, set the property `KuduMasters` to a CSV of the master addresses in the form `host:port` and add a table name value, as shown: > I meant the default you are using in your code. For example, by default you Done http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/README.adoc@43 PS4, Line 43: To specify a value at execution time, you'll specify the parameter name in the '-D<name>' format. For example, to set a different set of masters for the Kudu cluster from the command line and use a custom table name, set the property `KuduMasters` to a CSV of the master addresses in the form `host:port` and add a table name value, as shown: > Nit: line breaks. Done http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala File examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala: http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@16 PS4, Line 16: val KuduMasters: String = System.getProperty("KuduMasters","kudu.master1:7051,kudu.master2:7051,kudu.master3:7051") //kudu master address list > Can we use localhost:7051 as the default? Done http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@22 PS4, Line 22: //defining a class that we'll use to insert data into the table > Nit: periods at the end of comments. Done http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@27 PS4, Line 27: val logger = LoggerFactory.getLogger(SparkExample.getClass) > nit: this can be defined at the top of the object. Done http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@34 PS4, Line 34: import spark.implicits._ > Can you try moving this? It should be fine to be with the other imports. It fails to build: [ERROR] /Users/mbarnett/src/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala:14: error: not found: object spark [ERROR] import spark.implicits._ [ERROR] ^ [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE because it's using the 'spark' instance we declare above it, which actually contains the lib. http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@39 PS4, Line 39: List( > nit: This indentation seams level looks like it is using 4 spaces. Sorry, didn't quite understand this one. Should this be simply indented, or level? http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@40 PS4, Line 40: StructField(IdCol,IntegerType,false), > nit: space after commas. Done http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@48 PS4, Line 48: kc.createTable(TableName, schema, Seq(IdCol), new CreateTableOptions().setNumReplicas(3).addHashPartitions(List(IdCol).asJava, 3)) > This still has a replication factor of 3. Missed the change. Completed now. http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@71 PS4, Line 71: val upsertUsers = Array(User("newUserA", 1234), User("userC", 7777)) > To show the value of an upsert, should we re-use a key that was already ins Yep, I am repeating a value from above (the id=1234). This will upsert 1234 and change the name of 'userA' to 'newUserA'. If you think the change should be more drastic to call more attention to what the upsert did, let me know. I can have it change to something like "BrandNewUser" instead. http://gerrit.cloudera.org:8080/#/c/11788/4/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@84 PS4, Line 84: finally try { > just finally? Done -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 4 Gerrit-Owner: Mitch Barnett <mbarn...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett <mbarn...@cloudera.com> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 26 Oct 2018 14:43:13 +0000 Gerrit-HasComments: Yes
