Brian McDevitt has posted comments on this change. ( http://gerrit.cloudera.org:8080/15882 )
Change subject: [KUDU-3116] Enhance KuduContext row operation metrics ...................................................................... Patch Set 1: (6 comments) New patch set. Thanks for the review Andrew! http://gerrit.cloudera.org:8080/#/c/15882/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala: PS1: > nit: add the ASF license header Done http://gerrit.cloudera.org:8080/#/c/15882/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala@10 PS1, Line 10: class MapAccumulator[K, V](mergeFn: (V, V) => V) > Needs scaladoc string Done http://gerrit.cloudera.org:8080/#/c/15882/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala@14 PS1, Line 14: private val _map = Collections.synchronizedMap(new util.HashMap[K, V]()) > nit: seems like we don't use this style of private member in our scala code if, you're referring to the val name '_map', I agree and will rename. If not, then I'm not sure what specifically should be changed. http://gerrit.cloudera.org:8080/#/c/15882/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala@15 PS1, Line 15: _biFunc > nit: I think mergeFunc or mergeVals or something would be more descriptive Done http://gerrit.cloudera.org:8080/#/c/15882/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala@35 PS1, Line 35: override def merge(other: AccumulatorV2[(K, V), java.util.Map[K, V]]): Unit = other match { > nit: bring the match down to the next line, a la Done http://gerrit.cloudera.org:8080/#/c/15882/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/MapAccumulator.scala@49 PS1, Line 49: override def value: java.util.Map[K, V] = Collections.unmodifiableMap(_map) > I'm not too familiar with unmodifiableMap -- does this need to be wrapped i Good catch. This should be wrapped in a "_map.synchronized" block but I don't think callers need to synchronize on access. At least the CollectionAccumulator that is provided by Spark doesn't mention needing to synchronize access. -- To view, visit http://gerrit.cloudera.org:8080/15882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie66dab95041310c27ef62dacccbcc0977a84857e Gerrit-Change-Number: 15882 Gerrit-PatchSet: 1 Gerrit-Owner: Brian McDevitt <br...@phdata.io> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Brian McDevitt <br...@phdata.io> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 12 May 2020 16:45:55 +0000 Gerrit-HasComments: Yes
