Todd Lipcon has posted comments on this change.

Change subject: KUDU-1999: Spark connector should login with Kerberos 
credentials on driver
......................................................................


Patch Set 1:

(5 comments)

Can you add some color to the commit message on testing? Clearly our unit test 
environment isn't well equipped to test this right now, but would be good to 
leave some notes about how to test it

http://gerrit.cloudera.org:8080/#/c/6822/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:

Line 49: class KuduContext(kuduMaster: String, @transient sc: SparkContext) 
extends Serializable {
is there any way to magically get a SparkContext from a singleton or something?

If not, we'll need to document this in release notes and also update 
developing.adoc, where we show an example of creating a KuduContext by hand.

Perhaps a compatibility path where we can handle a null spark context is in 
order? Even though we say 'Unstable' above, we did go and document it, so I'd 
feel bad breaking it.


PS1, Line 59: loginUserFromKeytab
maybe rename to loginUser or getSubject or something, since many of the return 
paths don't login from keytab?


Line 62:     val principal = 
sc.getConf.getOption("spark.yarn.principal").getOrElse(return subject)
is principal always required? or is there some 'default' principal that would 
be applied?


PS1, Line 65:  if (subject != null && 
!subject.getPrincipals(classOf[KerberosPrincipal]).isEmpty) {
            :       return subject
            :     }
I'm wondering if this is always a good idea. If the user specifies a keytab, 
maybe we should always respect it? What if the one that is already in the 
Subject is an unrelated principal (eg from the wrong realm, etc)?


PS1, Line 295: private object KuduContext {
             :   val Log = LoggerFactory.getLogger(classOf[KuduContext])
             : }
is this Scala's equivalent of a static member?


-- 
To view, visit http://gerrit.cloudera.org:8080/6822
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If87a470c1cf99ea52668f22b72f1f7331877ec63
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to