[LIVY-494] Add thriftserver to Livy server #107
Conversation
|
kindly ping @jerryshao @vanzin |
| @@ -115,6 +117,15 @@ class LivyServer extends Logging { | |||
| error("Failed to run kinit, stopping the server.") | |||
| sys.exit(1) | |||
| } | |||
| // This is and should be the only place where a login() on the UGI is performed. Any | |||
There was a problem hiding this comment.
The comment you have in the PR description, that the current Hive Thrift code needs this, is way better here, since otherwise it's unclear why you need this login at all.
The rest of the comment is basically a long way of explaining the assert you added later in this class.
| @@ -134,6 +145,14 @@ class LivyServer extends Logging { | |||
| server = new WebServer(livyConf, host, port) | |||
| server.context.setResourceBase("src/main/org/apache/livy/server") | |||
|
|
|||
| def invokeOnStaticThriftserver(method: String, args: (Class[_], Object)*): Object = { | |||
There was a problem hiding this comment.
Instead of reflection hacks I'd use an interface to define a factory method to be called. You could use a java.util.ServiceLoader to load the class too, but with a single one, it's fine to use Class.forName.
| target.orElse(Option(remoteUser(req))) | ||
| } else { | ||
| None | ||
| protected def withHaltOnForbiddenAction(f: => S): S = { |
There was a problem hiding this comment.
Not a fan of the name; just haltOnForbiddenAction sounds better.
Also, it's been a long time since I looked at scalatra... but could you do this instead in the error handler of SessionServlet, and map the exception to the http error? Or is the "halt" required for some reason?
There was a problem hiding this comment.
I think it is doable. I'll try and do that. Thanks.
| accessManager: AccessManager): Option[String] = { | ||
| if (livyConf.getBoolean(LivyConf.IMPERSONATION_ENABLED)) { | ||
| if (!target.forall(hasSuperAccess(_, requestUser, accessManager))) { | ||
| throw new InvalidParameterException( |
There was a problem hiding this comment.
AccessControlException makes more sense here.
|
|
||
| object ThriftServerFactory { | ||
| def getInstance: ThriftServerFactory = { | ||
| Class.forName("org.apache.livy.thriftserver.LivyThriftServer$").getField("MODULE$").get(null) |
There was a problem hiding this comment.
Instead of this, have a ThriftServerFactoryImpl class that implements the start method, and instantiate it with Class.forName(blah).newInstance(). That's closer to what ServiceLoader does.
| * request's user - which may not be defined - otherwise, or `None` if impersonation is | ||
| * disabled. | ||
| */ | ||
| def checkImpersonation( |
There was a problem hiding this comment.
Since this method requires an accessManager, wouldn't it be better as a method in the AccessManager class itself? (Same for hasSuperAccess.)
There was a problem hiding this comment.
sure, I agree. I'll move hasViewAccess and hasModifyAccess too for coherency. Seems also that the AccessManager is the right place for putting the logic of these methods IMHO. Thanks.
| @@ -115,6 +117,16 @@ class LivyServer extends Logging { | |||
| error("Failed to run kinit, stopping the server.") | |||
| sys.exit(1) | |||
| } | |||
| // This is and should be the only place where a login() on the UGI is performed. | |||
There was a problem hiding this comment.
This should be pretty easy to check with a scalastyle rule that disallows calls to UserGroupInformation.login* (and disabling the check around this call). Not foolproof, but should catch inadvertent use at least.
There was a problem hiding this comment.
Actually this wouldn't really work as - for instance - UserGroupInformation.loginUserFromKeytabAndReturnUGI is ok to be called, as it doesn't perform a UserGroupInformation.login(). We can list all the APIs which we should not call, but I am not sure it is worth.
|
Travis failure is intermittent, the branch build passed: https://travis-ci.org/mgaido91/incubator-livy/builds/432380763 |
|
Merging to master. |
## What changes were proposed in this pull request? The PR adds a configuration parameter in order to startup also the thriftserver when starting Livy server. Apart from this trivial change, other 3 main things were needed and are present in this PR: - Add the thriftserver JARs to the assembly and the livy-server script; - A small refactor in order to enforce impersonation in the `*Session` classes, instead of in the `*Servlet` ones, so that it is picked up by the thriftserver module too (this change is not strictly needed, but I consider it a better option that duplicating this logic in the thriftserver module too); - Creating a UGI from the configured keytab. This is needed because the thriftserver requires the UGI to be created from a keytab in order to work properly and previously Livy was using a UGI generated from the cached TGT (created by the `kinit` command). ## How was this patch tested? Manual test: starting the server and having it up for more than 9 days. Author: Marco Gaido <mgaido@hortonworks.com> Closes apache#107 from mgaido91/LIVY-494.
What changes were proposed in this pull request?
The PR adds a configuration parameter in order to startup also the thriftserver when starting Livy server.
Apart from this trivial change, other 3 main things were needed and are present in this PR:
*Sessionclasses, instead of in the*Servletones, so that it is picked up by the thriftserver module too (this change is not strictly needed, but I consider it a better option that duplicating this logic in the thriftserver module too);kinitcommand).How was this patch tested?
Manual test: starting the server and having it up for more than 9 days.