[
https://issues.apache.org/jira/browse/HIVE-13445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15236393#comment-15236393
]
Siddharth Seth commented on HIVE-13445:
---------------------------------------
This would be easier to review on RB.
Bunch of comments meanwhile.
ContainerRunner, and the general API - can we avoid throwing an IOException,
and instead propagate either a RuntimeException or a SecurityException. I think
IOException is there primarily for handling errors from UGI. We're better of
catching those and sending them back as RuntimeExceptions. An IOException from
the RPC layer normally indicates a failure to communicate.
Nit: Random imports of Pair in 1-2 classes.
Any possibility of performing some basic sanity checks inside
LlapProtocolServerImpl - or is that already in place via the RPC layer
validating the presence of a LLAP token. Don't like the fact that the security
chceks are 3 calls deep - but that seems the best place for them rightnow.
generateClusterName - The regular expression and replacement - this is
duplicated somewhere else. Should be in a utility function.
String hostName = MetricsUtils.getHostName(); - Not necessarily related to this
patch, but getting it from YARN is more consistent (when yarn is available).
Have seen lots of issues around figuring out hostnames otherwise.
Figuring out the containerId - HIVE-13413 already adds this. It could be
re-used from there. In terms of the test failures - we've fixed such situations
before in Tez by having the core class accept parameters, and have the
environment read only in main methods. e.g. LlapDaemon reads this from env only
in main. MiniLlap sets it up since it creates LlapDaemon directly. (Unrelated:
separation between non-YARN clusters and YARN clusters - that's starting to
become problematic. Probably need an interface for the ClusterProvider similar
to the one in the registry)
{code}
LlapDeamon: appName = UUID.randomUUID().toString();
{code}
Ths won't work on distributed clusters, right ? Tokens use this as the
appSecret. Each node will generate a different appSecret. daemonId.getAppSecret
is being used as the clusterId in LlapTokenIdentifier.
LlapDeamon: new DeamonId - I think the host / appId fields are reversed.
In LlapTokenChecker - why are we iterating over tokens even after an LLAPToken
has been found ? Are multiple tokens expected. This is in checkPermissions as
well as getTokenInfo
{code}
if (appSecret != null && !userName.equals(newAppSecret))
{code}
userName !=newAppSecret ? Is this what it's supposed to be.
getPrmUserName
What does PRM stand for ? Use the full literal instead ?
QueryInfo.registerFragment.
It looks like we end up taking the first request and linking it with the query.
Also subsequent requests are validated against this. Assuming that this becomes
more useful once signing comes in - to make sure someone is not submitting
with incorrect parameters.
TaskExecutorService.findQueryByFragment - think we're better off implementing
this in QueryInfo itself rather than going to the scheduler to find out this
information. need to check if QueryInfo has state information about which
fragments are linked to a query.
getDelegationToken(String appSecret) - even in case of Tez, should this be
associated with the sessionId. That prevents a lot of the if (token.appSecret
== null) checks and will simplify the code.
I'm not sure we actually need to worry about user and realUser. Hive is not
running as a proxyUser. In fact a difference between the two will likely be an
error.
> LLAP: token should encode application and cluster ids
> -----------------------------------------------------
>
> Key: HIVE-13445
> URL: https://issues.apache.org/jira/browse/HIVE-13445
> Project: Hive
> Issue Type: Bug
> Reporter: Sergey Shelukhin
> Assignee: Sergey Shelukhin
> Attachments: HIVE-13445.01.patch, HIVE-13445.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
