Dan Burkert has posted comments on this change. Change subject: Spark connectors for Kudu ......................................................................
Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/1788/6/java/kudu-spark/src/main/scala/org/kududb/spark/DefaultSource.scala File java/kudu-spark/src/main/scala/org/kududb/spark/DefaultSource.scala: Line 20: import org.apache.spark.Logging > the Logging class isn't meant to a public part of spark (I think it was lea Done Line 41: val KUDU_MASTER: String = "kudu.master" > super nit, but its unusual to put in a type on simple declarations like thi Done Line 93: @transient lazy val kuduSchema = kuduTable.getSchema > looks like you only need the columns, so maybe better to use Done Line 95: kuduSchema.getColumns.asScala.map(c => (c.getName, c)).toMap > here the types aren't as obvious for kuduTable, kuduSchema, and kuduSchemaC Done http://gerrit.cloudera.org:8080/#/c/1788/6/java/kudu-spark/src/main/scala/org/kududb/spark/KuduContext.scala File java/kudu-spark/src/main/scala/org/kududb/spark/KuduContext.scala: Line 24: import org.apache.spark.{Logging, SparkContext} > same here on Logging Done Line 38: class KuduContext(sc: SparkContext, kuduMaster: String) extends Serializable with Logging { > We probably want to expose all of these functions via implicits, so its mor nixing all these, as per our conversation. Line 139: rdd.values > it would be nice if this rdd had its name set to something meaningful, eg. Done http://gerrit.cloudera.org:8080/#/c/1788/6/java/kudu-spark/src/test/scala/org/kududb/spark/DefaultSourceSuite.scala File java/kudu-spark/src/test/scala/org/kududb/spark/DefaultSourceSuite.scala: Line 19: import org.apache.spark.Logging > no need for Logging Done http://gerrit.cloudera.org:8080/#/c/1788/6/java/kudu-spark/src/test/scala/org/kududb/spark/TestContext.scala File java/kudu-spark/src/test/scala/org/kududb/spark/TestContext.scala: Line 53: sc = new SparkContext("local", "test", null, Nil, envMap) > not a big deal, but I'd suggest at least using "local[2]" in your test case Done Line 66: sc.stop() > nit: should all of these be inside a "if (sc != null)" etc. check, in case Done -- To view, visit http://gerrit.cloudera.org:8080/1788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic187513ef9724d50024f7401d7ecd19d53554245 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans Gerrit-Reviewer: Anonymous Coward #125 Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
