Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17328 )
Change subject: [spark] Add custom SASL protocol name ...................................................................... Patch Set 1: (2 comments) Looks reasonable to me, just one question and a nit. It would be great if you could get this reviewed by a Scala expert. http://gerrit.cloudera.org:8080/#/c/17328/1/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/17328/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@61 PS1, Line 61: @SerialVersionUID(1L) I'm not an expert in Scala, so I'm not sure whether the addition of 'saslProtocolName' means the SerialVersionUID should be bumped? http://gerrit.cloudera.org:8080/#/c/17328/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala: http://gerrit.cloudera.org:8080/#/c/17328/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@883 PS1, Line 883: KuduClientCache.clearCacheForTests() nit for here and below: even if it's semi-obvious, it would be great if you could add a comment explaining why it's necessary to call clearCacheForTests() -- To view, visit http://gerrit.cloudera.org:8080/17328 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifd0dba4f829f369c363cc89bb58650249035f356 Gerrit-Change-Number: 17328 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 21 Apr 2021 05:01:03 +0000 Gerrit-HasComments: Yes
