Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10839 )
Change subject: spark: Expose socketReadTimeoutMs to Spark connector ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala: http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala@188 PS3, Line 188: socketReadTimeoutMs match { : case Some(_) => context.socketReadTimeoutMs = socketReadTimeoutMs : case None => : } simplify this to just context.socketReadTimeoutMs = socketReadTimeoutMs http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@91 PS3, Line 91: var socketReadTimeoutMs: Option[Long] = None 'var' is wrong here -- if the caller gets the asyncClient, changes the read timeout and then gets the asyncClient again it will have the original value. I think it'd be better to add this as a val ctor param with a default value, i.e: class KuduContext(val kuduMaster: String, sc: SparkContext, sockedReadTimeoutMs: Option[Long] = None) extends Serializable { http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@111 PS3, Line 111: import kudu.KuduContext._ What is this for? I'd remove if possible, underscore imports are non-explicit in really tricky ways. http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@399 PS3, Line 399: @InterfaceAudience.Private These aren't necessary since it's a private object. -- To view, visit http://gerrit.cloudera.org:8080/10839 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ab0ff0b242790caffb7e2848958148ffe547c4d Gerrit-Change-Number: 10839 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 11 Jul 2018 19:05:58 +0000 Gerrit-HasComments: Yes
