Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 )
Change subject: [examples] Add basic Spark example written in Scala ...................................................................... Patch Set 1: (11 comments) Thanks for the contribution. I did a quick first review pass. 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@18 PS1, Line 18: = Kudu-Spark example README nit: extra space http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/README.adoc@40 PS1, Line 40: - ID_COL: String value containing the name of the Primary Key column. Is this needed? Given the code is creating the table, we have control over this. Same with NAME_COL. I think it complicates the example. Looking at the code, these aren't exposed as setable properties. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/README.adoc@43 PS1, Line 43: - SPARK_MASTER: String value which identifies the location of the Spark Master to be used. If running locally (standalone), this must be set to 'local'. nit: Move up near KUDU_MASTERS Can you specify if these have a default? http://gerrit.cloudera.org:8080/#/c/11788/1/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/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@14 PS1, Line 14: val KUDU_MASTERS : String = System.getProperty("KUDU_MASTERS","kudu.master1:7051,kudu.master2:7051,kudu.master3:7051") //kudu master address list nit: val KuduMasters: String = ... In Scala constants are usually named like objects. The others below should be changed too. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@19 PS1, Line 19: val SPARK_MASTER = "local" //location of spark master, specify 'local' if running on localhost Should this be configurable like KUDU_MASTERS? http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@32 PS1, Line 32: val schema : StructType = StructType { Nit: I think the below sytax is easier StructType( List( StructField("ID_COL", DataTypes.IntegerType,NULLABLE), StructField("NAME_COL", DataTypes.StringType,NULLABLE), )) ID columns can't be nullable. You can probably just hard coded if a column is nullable or not. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@39 PS1, Line 39: ID_COL nit: Replication factor of 1 might be easier for examples. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@43 PS1, Line 43: import spark.implicits._ Can this be moved up by the other imports. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@47 PS1, Line 47: System.out.println("Writing to table " + TABLE_NAME) Nit: Use slf4j logging. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@77 PS1, Line 77: case e: Exception => e.printStackTrace() No need to catch if there is no handling. Just let the exception bubble up. http://gerrit.cloudera.org:8080/#/c/11788/1/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@87 PS1, Line 87: case e: Exception => e.printStackTrace() No need to catch here either. -- 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: 1 Gerrit-Owner: Mitch Barnett <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 25 Oct 2018 19:46:01 +0000 Gerrit-HasComments: Yes
