> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, lines 2698-2699 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387290#file1387290line2698> > > > > Is this primarily for config ? Rename to have a positive connotation > > maybe ?
it says in the description :) > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java, > > line 29 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387292#file1387292line29> > > > > I'm not sure this will actually be usable, given that what is being > > signed is a protobuf generated class. It's used to implement a wrapper over protobuf > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java, > > line 134 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387295#file1387295line134> > > > > Can a second login be avoided. I'm guessing this is because the ZK > > principla may be different from the llap principla. > > What was the reason for them to be different again ? (Especially w.r.t > > the SecretManager). Not sure if the fallback to using the llap principal > > and keytab will work if they have to be different. The same principal didn't work on the test cluster I had for some reason that I no longer remember :( > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java, > > line 168 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387296#file1387296line168> > > > > Move this to after checking if vertexBinary is set ? Potentially error > > out if both are set. > > > > IIRC, vertexBinary will be set by external clients, and vertex will be > > set by Tez ? yes > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java, > > line 262 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387296#file1387296line262> > > > > Why is this required ? The signature will only exist if vertexBinary is > > present ? No reason why someone cannot set signature and vertex. > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java, > > line 170 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387296#file1387296line170> > > > > Maybe move all of these checks into the RPC layers itself ... i.e. > > LlapServiceServerImpl. As early as possible. The permissions are checked in calls by ContainerRunner. RPC right now just propagates the request... > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java, > > line 287 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387298#file1387298line287> > > > > All of this logic should be invoked even when obtaining tokens from > > ZKSM directly. > > > > Whether Tez is being used, or an external client - as long as HS2 is > > obtaining a token, it can do it directly from ZK. This code path is not > > likely to be exercised a lot. > > Assuming that invocation (when it happens, and likely needs another > > jira) - will call in to LlapTokenLocalClient.createToken directly - and > > will send in isSigningRequired based on all of the same configs. > > > > Would be better to move the logic out of this function in that case. > > > > Maybe the config flag itself could be dropped. If Tez, no singing, if > > external - force signing. this actually kind of orthogonal. This logic doesn't apply to when HS2 creates the token preciusely because HS2 knows whether it's creating the token for Tez or external, so it can set the flag accordingly. When the method is called remotely, by default we always require signing, but that can be disabled for CLI, or HS2 calling remotely (presumably under the same user as LLAP). > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java, > > line 290 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387298#file1387298line290> > > > > What user is expected over here. > > 1. In case of an invocation by HS2 to run a Tez query - I'm assuming > > this would be the HS2 service user (which is the same as the LLAP service > > user). (That needs to be validated) > > 2. In case of external services - would this be the HS2 service user or > > the user associated with the external service ? > > > > If it's the HS2 user each time, is the "user"/"realuser" field in the > > TokenIdentifier required ? That seems to be passed in as a null everywhere. > > Assuming the appId is what will be used to differentiate different > > external clients ? and that in case of Tez - there's no differentiation. This is the calling user in case of RPC. > On May 20, 2016, 2:21 a.m., Siddharth Seth wrote: > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java, > > line 61 > > <https://reviews.apache.org/r/46956/diff/2/?file=1387304#file1387304line61> > > > > I don't think we need to create a new instance of the > > ZKDelegationTokenSecretManager. > > > > The one created earlier to generate tokens should be passed in. > > > > The KeySigner could be an interface instead, and SecretManager (extends > > ZKDelegationTokenSecretManager) can implement this. ACL checks etc are > > already setup there. There's no requirement to have two independent copies > > of the ZKSecretManager running in the same daemon. This one has completely different logic and even different template parameter. - Sergey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46956/#review134076 ----------------------------------------------------------- On May 18, 2016, 8:36 p.m., Sergey Shelukhin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46956/ > ----------------------------------------------------------- > > (Updated May 18, 2016, 8:36 p.m.) > > > Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth. > > > Repository: hive-git > > > Description > ------- > > see jira > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java cbb3a72 > > llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java > f10351b > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java > e28eddd > > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java > PRE-CREATION > > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java > 465b204 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java > 2524dc2 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java > de817e3 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java > b94fc2e > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java > 03ee055 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java > 8abd198 > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java > eac0e8f > > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java > 74359fa > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java > PRE-CREATION > > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java > PRE-CREATION > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java > 279baf1 > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java > aaaa762 > > llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java > a250882 > ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b > > Diff: https://reviews.apache.org/r/46956/diff/ > > > Testing > ------- > > > Thanks, > > Sergey Shelukhin > >
