[GitHub] [incubator-livy] yantzu commented on issue #193: [LIVY-621]add dynamic service discovery for thrift server
yantzu commented on issue #193: [LIVY-621]add dynamic service discovery for thrift server URL: https://github.com/apache/incubator-livy/pull/193#issuecomment-540405424 Livy user in my company is asking for Rest API Load Balance functionality these days. So I go throught #189 and #212 and then I realize service discovery for Rest is quite different with Thrift. The main difference is thrift is a long connection while rest is not. When thrift connect is break, the session is shutdown but rest session is not. And another difference is thrift's client are exsiting hive client while rest have many different clients. These differences make thrift service discovery much easier than rest, so maybe we should consider them seperately. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server.
captainzmc commented on a change in pull request #231: [LIVY-356][SERVER]Add
LDAP authentication for livy-server.
URL: https://github.com/apache/incubator-livy/pull/231#discussion_r333415057
##
File path:
server/src/main/scala/org/apache/livy/server/auth/LdapAuthenticationHandlerImpl.scala
##
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.server.auth
+
+import java.io.IOException
+import java.nio.charset.StandardCharsets
+import java.util
+import java.util.Properties
+import javax.naming.NamingException
+import javax.naming.directory.InitialDirContext
+import javax.naming.ldap.{Control, InitialLdapContext, StartTlsRequest,
StartTlsResponse}
+import javax.net.ssl.{HostnameVerifier, SSLSession}
+import javax.servlet.ServletException
+import javax.servlet.http.{HttpServletRequest, HttpServletResponse}
+
+import org.apache.commons.codec.binary.Base64
+import org.apache.hadoop.security.authentication.client.AuthenticationException
+import
org.apache.hadoop.security.authentication.server.{AuthenticationHandler,
AuthenticationToken}
+
+import org.apache.livy._
+
+object LdapAuthenticationHandlerImpl {
+
+ val AUTHORIZATION_SCHEME = "Basic"
+ val TYPE = "ldap"
+ val SECURITY_AUTHENTICATION = "simple"
+ val PROVIDER_URL = "ldap.providerurl"
+ val BASE_DN = "ldap.basedn"
+ val LDAP_BIND_DOMAIN = "ldap.binddomain"
+ val ENABLE_START_TLS = "ldap.enablestarttls"
+
+ private def hasDomain(userName: String): Boolean = {
+indexOfDomainMatch(userName) > 0
+ }
+
+ /**
+* Get the index separating the user name from domain name (the user's name
up
+* to the first '/' or '@').
+*
+* @param userName full user name.
+* @return index of domain match or -1 if not found
+*/
+
+ private def indexOfDomainMatch(userName: String): Integer = {
+if (userName == null) {
+ -1
+} else {
+ val idx = userName.indexOf('/')
+ val idx2 = userName.indexOf('@')
+ // Use the earlier match.
+ var endIdx = Math.min(idx, idx2)
+
+ // Unless at least one of '/' or '@' was not found, in
+ // which case, user the latter match.
+ if (endIdx == -1) endIdx = Math.max(idx, idx2)
+ endIdx
+}
+ }
+}
+
+class LdapAuthenticationHandlerImpl extends AuthenticationHandler with Logging
{
+ private var ldapDomain = "null"
+ private var baseDN = "null"
+ private var providerUrl = "null"
+ private var enableStartTls = false
+ private var disableHostNameVerification = false
+
+ def getType: String = LdapAuthenticationHandlerImpl.TYPE
+
+ @throws[ServletException]
+ def init(config: Properties): Unit = {
+this.baseDN = config.getProperty(LdapAuthenticationHandlerImpl.BASE_DN)
+this.providerUrl =
config.getProperty(LdapAuthenticationHandlerImpl.PROVIDER_URL)
+this.ldapDomain =
config.getProperty(LdapAuthenticationHandlerImpl.LDAP_BIND_DOMAIN)
+this.enableStartTls =
config.getProperty(LdapAuthenticationHandlerImpl.ENABLE_START_TLS,
+ "false").toBoolean
+require(this.providerUrl != null, "The LDAP URI can not be null")
+
+if (this.enableStartTls.booleanValue) {
+ val tmp = this.providerUrl.toLowerCase
+ require(!tmp.startsWith("ldaps"), "Can not use ldaps and StartTLS option
at the same time")
+}
+ }
+
+ def destroy(): Unit = {
+ }
+
+ @throws[IOException]
+ @throws[AuthenticationException]
+ def managementOperation(token: AuthenticationToken, request:
HttpServletRequest,
+response: HttpServletResponse) : Boolean = true
+
+ @throws[IOException]
+ @throws[AuthenticationException]
+ def authenticate(request: HttpServletRequest,
+response: HttpServletResponse): AuthenticationToken = {
+var token: AuthenticationToken = null
+var authorization = request.getHeader("Authorization")
+
+if (authorization != null && authorization.regionMatches(true, 0,
+ LdapAuthenticationHandlerImpl.AUTHORIZATION_SCHEME, 0,
+ LdapAuthenticationHandlerImpl.AUTHORIZATION_SCHEME.length)) {
Review comment:
RegionMatch alone wasn't a good idea. Because NullPointerException when
authorization is null.In this case, we need to check that it is not null again.
It felt strange:
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333419265
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/auth/ldap/LdapUtils.scala
##
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.livy.thriftserver.auth.ldap
+
+import org.apache.commons.lang.StringUtils
+
+import org.apache.livy.{LivyConf, Logging}
+
+/**
+ * Static utility methods related to LDAP authentication module.
+ */
+object LdapUtils extends Logging{
+
+ /**
+* Extracts username from user DN.
+*
+* Examples:
+*
+* LdapUtils.extractUserName("UserName")= "UserName"
+* LdapUtils.extractUserName("usern...@mycorp.com") = "UserName"
+* LdapUtils.extractUserName("cn=UserName,dc=mycompany,dc=com") = "UserName"
+*
+*
+* @param userDn
+* @return
+*/
+ def extractUserName(userDn: String): String = {
+if (!isDn(userDn) && !hasDomain(userDn)) return userDn
+val domainIdx = indexOfDomainMatch(userDn)
+if (domainIdx > 0) return userDn.substring(0, domainIdx)
+if (userDn.contains("=")) return userDn.substring(userDn.indexOf("=") + 1,
userDn.indexOf(","))
+userDn
+ }
+
+ /**
+* Get the index separating the user name from domain name (the user's name
up
+* to the first '/' or '@').
+*
+* @param userName full user name.
+* @return index of domain match or -1 if not found
+*/
+ def indexOfDomainMatch(userName: String): Int = {
+if (userName == null) return -1
+val idx = userName.indexOf('/')
+val idx2 = userName.indexOf('@')
+var endIdx = Math.min(idx, idx2) // Use the earlier match.
+// Unless at least one of '/' or '@' was not found, in
+// which case, user the latter match.
+if (endIdx == -1) endIdx = Math.max(idx, idx2)
+endIdx
+ }
+
+ /**
+* Check for a domain part in the provided username.
+*
Review comment:
This is usually formatted when the API is displayed in idea
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333420717
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
Review comment:
It was removed in the previous commit
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333421978
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
Review comment:
It was removed in the previous commit
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server.
codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server. URL: https://github.com/apache/incubator-livy/pull/231#issuecomment-534468593 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=h1) Report > Merging [#231](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/0804c8ea8ece67d01ababec616c9ad8e3b15dc9f?src=pr=desc) will **decrease** coverage by `0.35%`. > The diff coverage is `50.8%`. [](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#231 +/- ## === - Coverage 68.45% 68.1% -0.36% - Complexity 927 939 +12 === Files 100 101 +1 Lines 57295853 +124 Branches870 886 +16 === + Hits 39223986 +64 - Misses 12471296 +49 - Partials560 571 +11 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.87% <0%> (-2.6%)` | `11 <0> (ø)` | | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.97% <100%> (+0.1%)` | `21 <0> (ø)` | :arrow_down: | | [...vy/server/auth/LdapAuthenticationHandlerImpl.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvYXV0aC9MZGFwQXV0aGVudGljYXRpb25IYW5kbGVySW1wbC5zY2FsYQ==) | `56.31% <56.31%> (ø)` | `13 <13> (?)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `55.88% <0%> (+2.94%)` | `7% <0%> (ø)` | :arrow_down: | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=footer). Last update [0804c8e...96199a3](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server.
codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server. URL: https://github.com/apache/incubator-livy/pull/231#issuecomment-534468593 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=h1) Report > Merging [#231](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/0804c8ea8ece67d01ababec616c9ad8e3b15dc9f?src=pr=desc) will **decrease** coverage by `0.35%`. > The diff coverage is `50.8%`. [](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#231 +/- ## === - Coverage 68.45% 68.1% -0.36% - Complexity 927 939 +12 === Files 100 101 +1 Lines 57295853 +124 Branches870 886 +16 === + Hits 39223986 +64 - Misses 12471296 +49 - Partials560 571 +11 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.87% <0%> (-2.6%)` | `11 <0> (ø)` | | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.97% <100%> (+0.1%)` | `21 <0> (ø)` | :arrow_down: | | [...vy/server/auth/LdapAuthenticationHandlerImpl.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvYXV0aC9MZGFwQXV0aGVudGljYXRpb25IYW5kbGVySW1wbC5zY2FsYQ==) | `56.31% <56.31%> (ø)` | `13 <13> (?)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `55.88% <0%> (+2.94%)` | `7% <0%> (ø)` | :arrow_down: | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=footer). Last update [0804c8e...96199a3](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] peter-toth commented on issue #243: [LIVY-693] Upgrade jackson to 2.9.10
peter-toth commented on issue #243: [LIVY-693] Upgrade jackson to 2.9.10 URL: https://github.com/apache/incubator-livy/pull/243#issuecomment-540411274 I think this is a maven cache issue on that particular machine. Only 1 job is failing with this build issue, the `Spark 2.2 Unit Tests`. Other jobs, including the `Spark 2.2 ITs` are ok and it has no issue with `scala-maven-plugin:3.2.2:compile (scala-compile-first) @ livy-api` phase. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] peter-toth edited a comment on issue #243: [LIVY-693] Upgrade jackson to 2.9.10
peter-toth edited a comment on issue #243: [LIVY-693] Upgrade jackson to 2.9.10 URL: https://github.com/apache/incubator-livy/pull/243#issuecomment-540411274 I think this is a maven cache issue on that particular machine. Only 1 job is failing with this build issue, the `Spark 2.2 Unit Tests`. Other jobs, including the `Spark 2.2 ITs` are ok and they have no issue with `scala-maven-plugin:3.2.2:compile (scala-compile-first) @ livy-api` phase. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r81184
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -122,31 +162,53 @@ class SparkYarnApp private[utils] (
with Logging {
import SparkYarnApp._
+ appList += this
+
private var killed = false
- private val appIdPromise: Promise[ApplicationId] = Promise()
private[utils] var state: SparkApp.State = SparkApp.State.STARTING
private var yarnDiagnostics: IndexedSeq[String] = IndexedSeq.empty[String]
+ private var appInfo = AppInfo()
+ private var appId: Option[ApplicationId] = None
override def log(): IndexedSeq[String] =
("stdout: " +:
process.map(_.inputLines).getOrElse(ArrayBuffer.empty[String])) ++
("\nstderr: " +:
process.map(_.errorLines).getOrElse(ArrayBuffer.empty[String])) ++
("\nYARN Diagnostics: " +: yarnDiagnostics)
+ private def getAppTag = appTag
+
override def kill(): Unit = synchronized {
killed = true
-if (isRunning) {
- try {
-val timeout = SparkYarnApp.getYarnTagToAppIdTimeout(livyConf)
-yarnClient.killApplication(Await.result(appIdPromise.future, timeout))
- } catch {
-// We cannot kill the YARN app without the app id.
-// There's a chance the YARN app hasn't been submitted during a
livy-server failure.
-// We don't want a stuck session that can't be deleted. Emit a warning
and move on.
-case _: TimeoutException | _: InterruptedException =>
- warn("Deleting a session while its YARN application is not found.")
- yarnAppMonitorThread.interrupt()
- } finally {
-process.foreach(_.destroy())
+
+if (!isRunning) {
+ return
+}
+
+process.foreach(_.destroy())
+
+if (!appId.isEmpty) {
+ yarnClient.killApplication(appId.get)
+ return
+} else {
+ Future {
Review comment:
Because the code in `Future` may cost a lot of time. Use `Future` can avoid
blocking the main thread.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333408607 ## File path: conf/livy.conf.template ## @@ -121,15 +121,21 @@ # If Livy can't find the yarn app within this time, consider it lost. # livy.server.yarn.app-lookup-timeout = 120s +# How long to find the yarn app +# livy.server.yarn.app-lookup-interval = 1s # When the cluster is busy, we may fail to launch yarn app in app-lookup-timeout, then it would # cause session leakage, so we need to check session leakage. # How long to check livy session leakage # livy.server.yarn.app-leakage.check-timeout = 600s # how often to check livy session leakage # livy.server.yarn.app-leakage.check-interval = 60s +# how long to monitor the yarn app +# livy.server.yarn.app-monitor.timeout = 1ms +# If Livy can't monitor the yarn app successfully within this max times, consider the app failed. +# livy.server.yarn.app-monitor.max-failed.times = 120 # How often Livy polls YARN to refresh YARN app state. -# livy.server.yarn.poll-interval = 5s +# livy.server.yarn.poll-interval = 500ms Review comment: Yes, I will revert to `5s`. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333408762
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -33,16 +33,23 @@ import org.apache.hadoop.yarn.conf.YarnConfiguration
import org.apache.hadoop.yarn.exceptions.ApplicationAttemptNotFoundException
import org.apache.hadoop.yarn.util.ConverterUtils
-import org.apache.livy.{LivyConf, Logging, Utils}
+import org.apache.livy.{LivyConf, Logging}
object SparkYarnApp extends Logging {
def init(livyConf: LivyConf): Unit = {
sessionLeakageCheckInterval =
livyConf.getTimeAsMs(LivyConf.YARN_APP_LEAKAGE_CHECK_INTERVAL)
sessionLeakageCheckTimeout =
livyConf.getTimeAsMs(LivyConf.YARN_APP_LEAKAGE_CHECK_TIMEOUT)
+yarnPollInterval = livyConf.getTimeAsMs(LivyConf.YARN_POLL_INTERVAL)
+yarnAppMonitorTimeout =
livyConf.getTimeAsMs(LivyConf.YARN_APP_MONITOR_TIMEOUT)
+yarnAppMonitorMaxFailedTimes =
livyConf.getTimeAsMs(LivyConf.YARN_APP_MONITOR_MAX_FAILED_TIMES)
+yarnAppLookUpInterval =
livyConf.getTimeAsMs(LivyConf.YARN_APP_LOOKUP_INTERVAL)
leakedAppsGCThread.setDaemon(true)
leakedAppsGCThread.setName("LeakedAppsGCThread")
leakedAppsGCThread.start()
+yarnAppMonitorThread.setDaemon(true)
Review comment:
Okay
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333418190
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/auth/ldap/LdapUtils.scala
##
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.livy.thriftserver.auth.ldap
+
+import org.apache.commons.lang.StringUtils
+
+import org.apache.livy.{LivyConf, Logging}
+
+/**
+ * Static utility methods related to LDAP authentication module.
+ */
+object LdapUtils extends Logging{
+
+ /**
+* Extracts username from user DN.
+*
+* Examples:
+*
+* LdapUtils.extractUserName("UserName")= "UserName"
+* LdapUtils.extractUserName("usern...@mycorp.com") = "UserName"
+* LdapUtils.extractUserName("cn=UserName,dc=mycompany,dc=com") = "UserName"
+*
+*
+* @param userDn
+* @return
+*/
+ def extractUserName(userDn: String): String = {
+if (!isDn(userDn) && !hasDomain(userDn)) return userDn
+val domainIdx = indexOfDomainMatch(userDn)
+if (domainIdx > 0) return userDn.substring(0, domainIdx)
+if (userDn.contains("=")) return userDn.substring(userDn.indexOf("=") + 1,
userDn.indexOf(","))
+userDn
+ }
+
+ /**
+* Get the index separating the user name from domain name (the user's name
up
+* to the first '/' or '@').
+*
+* @param userName full user name.
+* @return index of domain match or -1 if not found
+*/
+ def indexOfDomainMatch(userName: String): Int = {
+if (userName == null) return -1
+val idx = userName.indexOf('/')
+val idx2 = userName.indexOf('@')
+var endIdx = Math.min(idx, idx2) // Use the earlier match.
+// Unless at least one of '/' or '@' was not found, in
+// which case, user the latter match.
+if (endIdx == -1) endIdx = Math.max(idx, idx2)
+endIdx
+ }
+
+ /**
+* Check for a domain part in the provided username.
+*
+* Example:
+*
+*
+* LdapUtils.hasDomain("us...@mycorp.com") = true
+* LdapUtils.hasDomain("user1")= false
+*
+*
+* @param userName username
+* @return true if { @code userName} contains { @code @} part
+*/
+ def hasDomain(userName: String): Boolean = {
+indexOfDomainMatch(userName) > 0
+ }
+
+ /**
+* Detects DN names.
+*
+* Example:
+*
+*
+* LdapUtils.isDn("cn=UserName,dc=mycompany,dc=com") = true
+* LdapUtils.isDn("user1") = false
+*
+*
+* @param name name to be checked
+* @return true if the provided name is a distinguished name
+*/
+ def isDn(name: String): Boolean = {
+name.contains("=")
+ }
+
+ /**
+* Creates a principal to be used for user authentication.
+*
+* @param conf Livy configuration
+* @param user username
+* @return a list of user's principal
+*/
+ def createCandidatePrincipal(conf: LivyConf, user: String): String = {
+val ldapDomain = conf.get(LivyConf.THRIFT_LDAP_AUTHENTICATION_DOMAIN)
+val ldapBaseDN = conf.get(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN)
Review comment:
DN can be represented as a directory in ldap, for example: google.com, then
DN can be represented as: dc= Google,dc=com
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333409622
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -56,17 +63,24 @@ object SparkYarnApp extends Logging {
private def getYarnTagToAppIdTimeout(livyConf: LivyConf): FiniteDuration =
livyConf.getTimeAsMs(LivyConf.YARN_APP_LOOKUP_TIMEOUT) milliseconds
- private def getYarnPollInterval(livyConf: LivyConf): FiniteDuration =
-livyConf.getTimeAsMs(LivyConf.YARN_POLL_INTERVAL) milliseconds
-
private val appType = Set("SPARK").asJava
private val leakedAppTags = new
java.util.concurrent.ConcurrentHashMap[String, Long]()
+ private[utils] val appList = new ListBuffer[SparkYarnApp]()
Review comment:
Yes, I think it's better to support concurrent modification. I will change
it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333420717
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
Review comment:
It was removed in the previous commit
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
codecov-io edited a comment on issue #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain URL: https://github.com/apache/incubator-livy/pull/236#issuecomment-534452181 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=h1) Report > Merging [#236](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/0804c8ea8ece67d01ababec616c9ad8e3b15dc9f?src=pr=desc) will **decrease** coverage by `0.08%`. > The diff coverage is `100%`. [](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #236 +/- ## - Coverage 68.45% 68.37% -0.09% + Complexity 927 924 -3 Files 100 100 Lines 5729 5736 +7 Branches870 870 Hits 3922 3922 - Misses 1247 1251 +4 - Partials560 563 +3 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/236/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.01% <100%> (+0.14%)` | `21 <0> (ø)` | :arrow_down: | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/236/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.66% <0%> (-2.1%)` | `43% <0%> (-3%)` | | | [...c/src/main/java/org/apache/livy/rsc/RSCClient.java](https://codecov.io/gh/apache/incubator-livy/pull/236/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9SU0NDbGllbnQuamF2YQ==) | `72.28% <0%> (-1.21%)` | `20% <0%> (ø)` | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=footer). Last update [0804c8e...860728c](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
codecov-io edited a comment on issue #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain URL: https://github.com/apache/incubator-livy/pull/236#issuecomment-534452181 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=h1) Report > Merging [#236](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/0804c8ea8ece67d01ababec616c9ad8e3b15dc9f?src=pr=desc) will **decrease** coverage by `0.08%`. > The diff coverage is `100%`. [](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #236 +/- ## - Coverage 68.45% 68.37% -0.09% + Complexity 927 924 -3 Files 100 100 Lines 5729 5736 +7 Branches870 870 Hits 3922 3922 - Misses 1247 1251 +4 - Partials560 563 +3 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/236/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.01% <100%> (+0.14%)` | `21 <0> (ø)` | :arrow_down: | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/236/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.66% <0%> (-2.1%)` | `43% <0%> (-3%)` | | | [...c/src/main/java/org/apache/livy/rsc/RSCClient.java](https://codecov.io/gh/apache/incubator-livy/pull/236/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9SU0NDbGllbnQuamF2YQ==) | `72.28% <0%> (-1.21%)` | `20% <0%> (ø)` | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=footer). Last update [0804c8e...860728c](https://codecov.io/gh/apache/incubator-livy/pull/236?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333407101
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -98,7 +112,33 @@ object SparkYarnApp extends Logging {
}
}
-
+ private val yarnAppMonitorThread = new Thread() {
+override def run() : Unit = {
+ val executor = Executors.newSingleThreadExecutor
+ while (true) {
+for (app <- appList) {
+ val handler = executor.submit(new Runnable {
Review comment:
The Future thread cannot be interrupted when timeout.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333403822
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -165,40 +227,24 @@ class SparkYarnApp private[utils] (
/**
* Find the corresponding YARN application id from an application tag.
*
- * @param appTag The application tag tagged on the target application.
- * If the tag is not unique, it returns the first application
it found.
- * It will be converted to lower case to match YARN's
behaviour.
* @return ApplicationId or the failure.
*/
- @tailrec
- private def getAppIdFromTag(
- appTag: String,
- pollInterval: Duration,
- deadline: Deadline): ApplicationId = {
-if (isProcessErrExit()) {
- throw new IllegalStateException("spark-submit start failed")
-}
-
-val appTagLowerCase = appTag.toLowerCase()
-
-// FIXME Should not loop thru all YARN applications but YarnClient doesn't
offer an API.
-// Consider calling rmClient in YarnClient directly.
-
yarnClient.getApplications(appType).asScala.find(_.getApplicationTags.contains(appTagLowerCase))
-match {
- case Some(app) => app.getApplicationId
- case None =>
-if (deadline.isOverdue) {
- process.foreach(_.destroy())
- leakedAppTags.put(appTag, System.currentTimeMillis())
+ private def getAppId(): ApplicationId = {
+appIdOption.map(ConverterUtils.toApplicationId).getOrElse {
+ val appTagLowerCase = appTag.toLowerCase()
+ // FIXME Should not loop thru all YARN applications but YarnClient
doesn't offer an API.
+ // Consider calling rmClient in YarnClient directly.
+ yarnClient.getApplications(appType).asScala.
+find(_.getApplicationTags.contains(appTagLowerCase))
+ match {
+case Some(app) => app.getApplicationId
+case None =>
throw new IllegalStateException(s"No YARN application is found with
tag" +
s" $appTagLowerCase in
${livyConf.getTimeAsMs(LivyConf.YARN_APP_LOOKUP_TIMEOUT)/1000}" +
" seconds. This may be because 1) spark-submit fail to submit
application to YARN; " +
"or 2) YARN cluster doesn't have enough resources to start the
application in time. " +
"Please check Livy log and YARN log to know the details.")
-} else {
- Clock.sleep(pollInterval.toMillis)
- getAppIdFromTag(appTagLowerCase, pollInterval, deadline)
Review comment:
Because this code call `getAppIdFromTag` recursively until successful or
`deadline.isOverdue`, which maybe cost a lot of time, and delay to monitor
other app. In this pr, if fail to `getAppId`, end the monitor of current app,
and monitor the next app. If the times of failing to `getAppId` of appX exceed
the threshold, change the appX state to failed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] huianyi commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
huianyi commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333412835
##
File path: core/src/main/scala/org/apache/livy/OperationMessageManager.scala
##
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+object OperationMessageManager extends Logging{
+
+ def offer(message: Any): Unit = {
+if (threadLocalLogQueue.get() != null){
+ threadLocalLogQueue.get().offer(message.toString)
+}
+ }
+
+ def get(): ConcurrentBoundedLinkedQueue[String] = {
+threadLocalLogQueue.get()
+ }
+
+ def set(threadLocalLogQueue: ConcurrentBoundedLinkedQueue[String]){
+this.threadLocalLogQueue.set(threadLocalLogQueue)
+ }
+ private val threadLocalLogQueue = new
ThreadLocal[ConcurrentBoundedLinkedQueue[String]]
Review comment:
I think this gives us another more convenient way to get operationMessage
object in other classes if they are in the same thread(just like
`Sessionstate.get()` method in hive), for those classes are not in the same
thread, we need deliver this object to them, like Class
`LivyExecuteStatementOperation`.
Actually, if we do not use this threadLocal variable, for class
`ClientProtocol` needing operationMessage to deliver process message, we should
first deliver operationMessage to RSCClient, then RSCClient deliver it to
ClientProtocol, which needs lots of set and get method.
Would it be aIright if I add detailed comment in `OperationMessageManager`
to call developers' attention to the constraint?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333422091
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
+try {
+ handler = new LdapAuthenticationProviderImpl(livyConf)
+ handler.Authenticate(user, pwd)
+} catch {
+ case e: AuthenticationException =>
+val message = String.format("Authentication failed for user '%s' with
password '%s'",
+ user, pwd)
+throw new AssertionError(message, e)
+}
+ }
+
+ @Test(timeout = 6)
+ @throws[Exception]
+ def testAuthenticateWithWrongUser(): Unit = {
+
Review comment:
It was removed in the previous commit
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333421978
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
Review comment:
It was removed in the previous commit
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
yiheng commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r78399
##
File path: core/src/main/scala/org/apache/livy/OperationMessageManager.scala
##
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+object OperationMessageManager extends Logging{
+
+ def offer(message: Any): Unit = {
+if (threadLocalLogQueue.get() != null){
+ threadLocalLogQueue.get().offer(message.toString)
+}
+ }
+
+ def get(): ConcurrentBoundedLinkedQueue[String] = {
+threadLocalLogQueue.get()
+ }
+
+ def set(threadLocalLogQueue: ConcurrentBoundedLinkedQueue[String]){
+this.threadLocalLogQueue.set(threadLocalLogQueue)
+ }
+ private val threadLocalLogQueue = new
ThreadLocal[ConcurrentBoundedLinkedQueue[String]]
Review comment:
In the current implementation, it should be safe.
But we bring in an implicit constraint on how to use the
`OperationMessageManager.get()` and `OperationMessageManager.set()` methods,
that you must make sure they're called in the same thread. How can we guarantee
developer will not break it in the future?
IMHO, compared to the old code, it's not straightforward to understand and
maintain.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333423403
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
+try {
+ handler = new LdapAuthenticationProviderImpl(livyConf)
+ handler.Authenticate(user, pwd)
+} catch {
+ case e: AuthenticationException =>
+val message = String.format("Authentication failed for user '%s' with
password '%s'",
+ user, pwd)
+throw new AssertionError(message, e)
+}
+ }
+
+ @Test(timeout = 6)
+ @throws[Exception]
+ def testAuthenticateWithWrongUser(): Unit = {
+
Review comment:
A blank line is required between methods
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333424085
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
Review comment:
Method needs appropriate blank lines to make the logic clearer
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333403822
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -165,40 +227,24 @@ class SparkYarnApp private[utils] (
/**
* Find the corresponding YARN application id from an application tag.
*
- * @param appTag The application tag tagged on the target application.
- * If the tag is not unique, it returns the first application
it found.
- * It will be converted to lower case to match YARN's
behaviour.
* @return ApplicationId or the failure.
*/
- @tailrec
- private def getAppIdFromTag(
- appTag: String,
- pollInterval: Duration,
- deadline: Deadline): ApplicationId = {
-if (isProcessErrExit()) {
- throw new IllegalStateException("spark-submit start failed")
-}
-
-val appTagLowerCase = appTag.toLowerCase()
-
-// FIXME Should not loop thru all YARN applications but YarnClient doesn't
offer an API.
-// Consider calling rmClient in YarnClient directly.
-
yarnClient.getApplications(appType).asScala.find(_.getApplicationTags.contains(appTagLowerCase))
-match {
- case Some(app) => app.getApplicationId
- case None =>
-if (deadline.isOverdue) {
- process.foreach(_.destroy())
- leakedAppTags.put(appTag, System.currentTimeMillis())
+ private def getAppId(): ApplicationId = {
+appIdOption.map(ConverterUtils.toApplicationId).getOrElse {
+ val appTagLowerCase = appTag.toLowerCase()
+ // FIXME Should not loop thru all YARN applications but YarnClient
doesn't offer an API.
+ // Consider calling rmClient in YarnClient directly.
+ yarnClient.getApplications(appType).asScala.
+find(_.getApplicationTags.contains(appTagLowerCase))
+ match {
+case Some(app) => app.getApplicationId
+case None =>
throw new IllegalStateException(s"No YARN application is found with
tag" +
s" $appTagLowerCase in
${livyConf.getTimeAsMs(LivyConf.YARN_APP_LOOKUP_TIMEOUT)/1000}" +
" seconds. This may be because 1) spark-submit fail to submit
application to YARN; " +
"or 2) YARN cluster doesn't have enough resources to start the
application in time. " +
"Please check Livy log and YARN log to know the details.")
-} else {
- Clock.sleep(pollInterval.toMillis)
- getAppIdFromTag(appTagLowerCase, pollInterval, deadline)
Review comment:
Because this code call `getAppIdFromTag` recursively until successful or
`deadline.isOverdue`, which maybe cost a lot of time, and delay to monitor
other app.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333424915
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/auth/ldap/LdapUtils.scala
##
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.livy.thriftserver.auth.ldap
+
+import org.apache.commons.lang.StringUtils
+
+import org.apache.livy.{LivyConf, Logging}
+
+/**
+ * Static utility methods related to LDAP authentication module.
+ */
+object LdapUtils extends Logging{
+
+ /**
+* Extracts username from user DN.
+*
+* Examples:
+*
+* LdapUtils.extractUserName("UserName")= "UserName"
+* LdapUtils.extractUserName("usern...@mycorp.com") = "UserName"
+* LdapUtils.extractUserName("cn=UserName,dc=mycompany,dc=com") = "UserName"
+*
+*
+* @param userDn
+* @return
+*/
+ def extractUserName(userDn: String): String = {
+if (!isDn(userDn) && !hasDomain(userDn)) return userDn
+val domainIdx = indexOfDomainMatch(userDn)
+if (domainIdx > 0) return userDn.substring(0, domainIdx)
+if (userDn.contains("=")) return userDn.substring(userDn.indexOf("=") + 1,
userDn.indexOf(","))
+userDn
+ }
+
+ /**
+* Get the index separating the user name from domain name (the user's name
up
+* to the first '/' or '@').
+*
+* @param userName full user name.
+* @return index of domain match or -1 if not found
+*/
+ def indexOfDomainMatch(userName: String): Int = {
+if (userName == null) return -1
+val idx = userName.indexOf('/')
+val idx2 = userName.indexOf('@')
+var endIdx = Math.min(idx, idx2) // Use the earlier match.
+// Unless at least one of '/' or '@' was not found, in
+// which case, user the latter match.
+if (endIdx == -1) endIdx = Math.max(idx, idx2)
+endIdx
+ }
+
+ /**
+* Check for a domain part in the provided username.
+*
+* Example:
+*
+*
+* LdapUtils.hasDomain("us...@mycorp.com") = true
+* LdapUtils.hasDomain("user1")= false
+*
+*
+* @param userName username
+* @return true if { @code userName} contains { @code @} part
+*/
+ def hasDomain(userName: String): Boolean = {
+indexOfDomainMatch(userName) > 0
+ }
+
+ /**
+* Detects DN names.
+*
+* Example:
+*
+*
+* LdapUtils.isDn("cn=UserName,dc=mycompany,dc=com") = true
+* LdapUtils.isDn("user1") = false
+*
+*
+* @param name name to be checked
+* @return true if the provided name is a distinguished name
+*/
+ def isDn(name: String): Boolean = {
Review comment:
This format may not be recommended
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] huianyi commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
huianyi commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r68731
##
File path: core/src/main/scala/org/apache/livy/OperationMessageManager.scala
##
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+object OperationMessageManager extends Logging{
+
+ def offer(message: Any): Unit = {
+if (threadLocalLogQueue.get() != null){
+ threadLocalLogQueue.get().offer(message.toString)
+}
+ }
+
+ def get(): ConcurrentBoundedLinkedQueue[String] = {
+threadLocalLogQueue.get()
+ }
+
+ def set(threadLocalLogQueue: ConcurrentBoundedLinkedQueue[String]){
+this.threadLocalLogQueue.set(threadLocalLogQueue)
+ }
+ private val threadLocalLogQueue = new
ThreadLocal[ConcurrentBoundedLinkedQueue[String]]
Review comment:
Using this threadLocal variable gives us an easy way to get operationMessage
object in other classes, we do not need transfer it in other classes. For
example, Class ClientProtocol need operationMessage to deliver process message
to the end user, we can just call `OperationMessageManager.get()` to get it.
```java
private final ConcurrentBoundedLinkedQueue operationMessages =
OperationMessageManager.get();
```
I don't think this might be reused of another rpc client,
ConcurrentBoundedLinkedQueue would be initialized in
`LivyThriftSessionManager.openSession` method, for every new session, we will
create only one RSCClient for it, and both of them are in the same thread, so
for every RSCClient, they will have the right ConcurrentBoundedLinkedQueue.
Please correct me if I miss something.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] huianyi commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
huianyi commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r68731
##
File path: core/src/main/scala/org/apache/livy/OperationMessageManager.scala
##
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+object OperationMessageManager extends Logging{
+
+ def offer(message: Any): Unit = {
+if (threadLocalLogQueue.get() != null){
+ threadLocalLogQueue.get().offer(message.toString)
+}
+ }
+
+ def get(): ConcurrentBoundedLinkedQueue[String] = {
+threadLocalLogQueue.get()
+ }
+
+ def set(threadLocalLogQueue: ConcurrentBoundedLinkedQueue[String]){
+this.threadLocalLogQueue.set(threadLocalLogQueue)
+ }
+ private val threadLocalLogQueue = new
ThreadLocal[ConcurrentBoundedLinkedQueue[String]]
Review comment:
Using this threadLocal variable gives us an easy way to get operationMessage
object in other classes, we do not need transfer it in other classes. For
example, Class ClientProtocol need operationMessage to deliver process message
to the end user, we can just call `OperationMessageManager.get()` to get it.
```java
private final ConcurrentBoundedLinkedQueue operationMessages =
OperationMessageManager.get();
```
I don't think this might be reused for another rpc client,
ConcurrentBoundedLinkedQueue would be initialized in
`LivyThriftSessionManager.openSession` method, for every new session, we will
create only one RSCClient for it, and both of them are in the same thread, so
for every RSCClient, they will have the right ConcurrentBoundedLinkedQueue.
Please correct me if I miss something.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap authentication, based on ldapurl, basedn, domain
captainzmc commented on a change in pull request #236: [LIVY-678] Thrift ldap
authentication, based on ldapurl, basedn, domain
URL: https://github.com/apache/incubator-livy/pull/236#discussion_r333422091
##
File path:
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/auth/TestLdapAuthenticationProviderImpl.scala
##
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.thriftserver.auth
+
+import javax.security.sasl.AuthenticationException
+
+import org.apache.directory.server.annotations.CreateLdapServer
+import org.apache.directory.server.annotations.CreateTransport
+import org.apache.directory.server.core.annotations.ApplyLdifs
+import org.apache.directory.server.core.annotations.ContextEntry
+import org.apache.directory.server.core.annotations.CreateDS
+import org.apache.directory.server.core.annotations.CreatePartition
+import org.apache.directory.server.core.integ.AbstractLdapTestUnit
+import org.apache.directory.server.core.integ.FrameworkRunner
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+import org.apache.livy.LivyConf
+
+/**
+ * This unit test verifies the functionality of
LdapAuthenticationProviderImpl.
+ */
+@RunWith(classOf[FrameworkRunner])
+@CreateLdapServer(transports = Array(new CreateTransport(protocol = "LDAP",
address = "localhost")))
+@CreateDS(allowAnonAccess = true, partitions = Array(new CreatePartition(name
= "Test_Partition",
+ suffix = "dc=example,dc=com", contextEntry = new ContextEntry(entryLdif =
"dn: dc=example," +
+"dc=com \ndc: example\nobjectClass: top\n" + "objectClass: domain\n\n"
+@ApplyLdifs(Array("dn: uid=bjones,dc=example,dc=com", "cn: Bob Jones", "sn:
Jones",
+ "objectClass: inetOrgPerson", "uid: bjones", "userPassword: p@ssw0rd"))
+class TestLdapAuthenticationProviderImpl extends AbstractLdapTestUnit {
+ private var handler: LdapAuthenticationProviderImpl = null
+ private var user = "bjones"
+ private var pwd = "p@ssw0rd"
+ val livyConf = new LivyConf()
+
+ @Before
+ @throws[Exception]
+ def setup(): Unit = {
+livyConf.set(LivyConf.THRIFT_AUTHENTICATION, "ldap")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_BASEDN,
"dc=example,dc=com")
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_URL,
String.format("ldap://%s:%s;, "localhost",
+ AbstractLdapTestUnit.getLdapServer.getPort.toString))
+livyConf.set(LivyConf.THRIFT_LDAP_AUTHENTICATION_USERFILTER, "bjones,jake")
+ }
+
+ @Test(timeout = 6)
+ @throws[AuthenticationException]
+ def testAuthenticatePasses(): Unit = {
+
+try {
+ handler = new LdapAuthenticationProviderImpl(livyConf)
+ handler.Authenticate(user, pwd)
+} catch {
+ case e: AuthenticationException =>
+val message = String.format("Authentication failed for user '%s' with
password '%s'",
+ user, pwd)
+throw new AssertionError(message, e)
+}
+ }
+
+ @Test(timeout = 6)
+ @throws[Exception]
+ def testAuthenticateWithWrongUser(): Unit = {
+
Review comment:
It was removed in the previous commit
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333407101
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -98,7 +112,33 @@ object SparkYarnApp extends Logging {
}
}
-
+ private val yarnAppMonitorThread = new Thread() {
+override def run() : Unit = {
+ val executor = Executors.newSingleThreadExecutor
+ while (true) {
+for (app <- appList) {
+ val handler = executor.submit(new Runnable {
Review comment:
The Scala Future thread cannot be interrupted when timeout.
https://stackoverflow.com/questions/16009837/how-to-cancel-future-in-scala
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333407101
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -98,7 +112,33 @@ object SparkYarnApp extends Logging {
}
}
-
+ private val yarnAppMonitorThread = new Thread() {
+override def run() : Unit = {
+ val executor = Executors.newSingleThreadExecutor
+ while (true) {
+for (app <- appList) {
+ val handler = executor.submit(new Runnable {
Review comment:
The Future thread cannot be interrupted when timeout.
https://stackoverflow.com/questions/16009837/how-to-cancel-future-in-scala
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn
runzhiwang commented on a change in pull request #242: [LIVY-336] Livy should
not spawn one thread per job to track the job on Yarn
URL: https://github.com/apache/incubator-livy/pull/242#discussion_r333407101
##
File path: server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala
##
@@ -98,7 +112,33 @@ object SparkYarnApp extends Logging {
}
}
-
+ private val yarnAppMonitorThread = new Thread() {
+override def run() : Unit = {
+ val executor = Executors.newSingleThreadExecutor
+ while (true) {
+for (app <- appList) {
+ val handler = executor.submit(new Runnable {
Review comment:
The Scala Future thread cannot be interrupted when timeout, which maybe
caused by rpc call.
https://stackoverflow.com/questions/16009837/how-to-cancel-future-in-scala
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] peter-toth opened a new pull request #243: [LIVY-693] Upgrade jackson to 2.9.10
peter-toth opened a new pull request #243: [LIVY-693] Upgrade jackson to 2.9.10 URL: https://github.com/apache/incubator-livy/pull/243 ## What changes were proposed in this pull request? Upgrade Jackson to 2.9.10 which fixes many CVEs: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.9.10 ## How was this patch tested? Existing UTs. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] jerryshao closed pull request #243: [LIVY-693] Upgrade jackson to 2.9.10
jerryshao closed pull request #243: [LIVY-693] Upgrade jackson to 2.9.10 URL: https://github.com/apache/incubator-livy/pull/243 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333805180
##
File path: core/src/main/scala/org/apache/livy/OperationMessageManager.scala
##
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+object OperationMessageManager extends Logging{
+
+ def offer(message: Any): Unit = {
+if (threadLocalLogQueue.get() != null){
+ threadLocalLogQueue.get().offer(message.toString)
+}
+ }
+
+ def get(): ConcurrentBoundedLinkedQueue[String] = {
+threadLocalLogQueue.get()
+ }
+
+ def set(threadLocalLogQueue: ConcurrentBoundedLinkedQueue[String]){
+this.threadLocalLogQueue.set(threadLocalLogQueue)
+ }
+ private val threadLocalLogQueue = new
ThreadLocal[ConcurrentBoundedLinkedQueue[String]]
Review comment:
Yes, it is strange to use threadlocal variable here? I would suggest to a
more decent way even if we need more code changes.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on issue #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on issue #238: [LIVY-689] Deliver stage process message to the end user using thriftserver URL: https://github.com/apache/incubator-livy/pull/238#issuecomment-540876001 > Beeline from Hive 2.2 has in-place progress bar hive.server2.in.place.progress > >https://cwiki.apache.org/confluence/display/Hive/Configuration+Properties#ConfigurationProperties-hive.server2.in.place.progress > >(I think it works only with tez - would be nice if this would work with Livy too ) Would be better to support this if it is not a big effort. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333808458
##
File path:
core/src/main/scala/org/apache/livy/ConcurrentBoundedLinkedQueue.scala
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+import java.util.concurrent.ConcurrentLinkedQueue
+
+class ConcurrentBoundedLinkedQueue[E](capacity: Long) extends
ConcurrentLinkedQueue[E]{
+
+ override def offer(e: E): Boolean = {
+if (this.size() == capacity){
Review comment:
I guess there has two spaces before `capacity`
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333809766
##
File path: rsc/src/main/java/org/apache/livy/rsc/driver/RSCDriver.java
##
@@ -212,6 +215,33 @@ public void onSaslComplete(Rpc client) {
setupIdleTimeout();
}
+ /**
+ * calculate each stage's process and broadcast message to the end user
+ * @param jobId
+ */
+ void handleProcessMessage(String jobId){
+SparkStatusTracker sparkStatusTracker;
+synchronized (jcLock){
+ if (jc == null){
+return;
+ }
+ sparkStatusTracker = jc.sc().sc().statusTracker();
+}
+int[] activeStageIds = sparkStatusTracker.getActiveStageIds();
Review comment:
I also have the same concern. Beeline can only submit one query a time, but
what if user submits query programmatically? We can hardly say that all the
active stages belong to one query, so there should be a way to figure out job
id -> active stages mapping.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333805483
##
File path: rsc/src/main/java/org/apache/livy/rsc/BaseProtocol.java
##
@@ -126,6 +126,28 @@ public JobResult() {
}
+ protected static class JobProcessMessage {
+public final String id;
+public final int stageId;
+public final int completed;
+public final int all;
+public final int active;
+public final int failed;
+public JobProcessMessage(String id, int stageId, int completed, int active,
Review comment:
Please change the style to one parameter per line.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333808344
##
File path:
core/src/main/scala/org/apache/livy/ConcurrentBoundedLinkedQueue.scala
##
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy
+
+import java.util.concurrent.ConcurrentLinkedQueue
+
+class ConcurrentBoundedLinkedQueue[E](capacity: Long) extends
ConcurrentLinkedQueue[E]{
Review comment:
Space before `{`. Please fix here and various other places.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server.
codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server. URL: https://github.com/apache/incubator-livy/pull/231#issuecomment-534468593 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=h1) Report > Merging [#231](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/0804c8ea8ece67d01ababec616c9ad8e3b15dc9f?src=pr=desc) will **decrease** coverage by `0.39%`. > The diff coverage is `50.8%`. [](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #231 +/- ## === - Coverage 68.45% 68.06% -0.4% - Complexity 927 939 +12 === Files 100 101 +1 Lines 5729 5853+124 Branches870 886 +16 === + Hits 3922 3984 +62 - Misses 1247 1297 +50 - Partials560 572 +12 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.33% <0%> (-2.14%)` | `11 <0> (ø)` | | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.97% <100%> (+0.1%)` | `21 <0> (ø)` | :arrow_down: | | [...vy/server/auth/LdapAuthenticationHandlerImpl.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvYXV0aC9MZGFwQXV0aGVudGljYXRpb25IYW5kbGVySW1wbC5zY2FsYQ==) | `56.31% <56.31%> (ø)` | `13 <13> (?)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `79.91% <0%> (-0.84%)` | `45% <0%> (-1%)` | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=footer). Last update [0804c8e...62d8fa0](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server.
jerryshao commented on a change in pull request #231: [LIVY-356][SERVER]Add
LDAP authentication for livy-server.
URL: https://github.com/apache/incubator-livy/pull/231#discussion_r333802329
##
File path:
server/src/main/scala/org/apache/livy/server/auth/LdapAuthenticationHandlerImpl.scala
##
@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.livy.server.auth
+
+import java.io.IOException
+import java.nio.charset.StandardCharsets
+import java.util
+import java.util.Properties
+import javax.naming.NamingException
+import javax.naming.directory.InitialDirContext
+import javax.naming.ldap.{InitialLdapContext, StartTlsRequest,
StartTlsResponse}
+import javax.net.ssl.{HostnameVerifier, SSLSession}
+import javax.servlet.ServletException
+import javax.servlet.http.{HttpServletRequest, HttpServletResponse}
+
+import org.apache.commons.codec.binary.Base64
+import org.apache.hadoop.security.authentication.client.AuthenticationException
+import
org.apache.hadoop.security.authentication.server.{AuthenticationHandler,
AuthenticationToken}
+
+import org.apache.livy._
+
+object LdapAuthenticationHandlerImpl {
+
+ val AUTHORIZATION_SCHEME = "Basic"
+ val TYPE = "ldap"
+ val SECURITY_AUTHENTICATION = "simple"
+ val PROVIDER_URL = "ldap.providerurl"
+ val BASE_DN = "ldap.basedn"
+ val LDAP_BIND_DOMAIN = "ldap.binddomain"
+ val ENABLE_START_TLS = "ldap.enablestarttls"
+
+ private def hasDomain(userName: String): Boolean = {
+indexOfDomainMatch(userName) > 0
+ }
+
+ /**
+* Get the index separating the user name from domain name (the user's name
up
+* to the first '/' or '@').
+*/
+ private def indexOfDomainMatch(userName: String): Int = {
+val idx = userName.indexOf('/')
+val idx2 = userName.indexOf('@')
+// Use the earlier match.
+var endIdx = Math.min(idx, idx2)
+
+// If neither '/' nor '@' was found, using the latter
+if (endIdx == -1) Math.max(idx, idx2) else endIdx
+ }
+}
+
+class LdapAuthenticationHandlerImpl extends AuthenticationHandler with Logging
{
+ private var ldapDomain = "null"
+ private var baseDN = "null"
+ private var providerUrl = "null"
+ private var enableStartTls = false
+ private var disableHostNameVerification = false
+
+ def getType: String = LdapAuthenticationHandlerImpl.TYPE
+
+ @throws[ServletException]
+ def init(config: Properties): Unit = {
+this.baseDN = config.getProperty(LdapAuthenticationHandlerImpl.BASE_DN)
+this.providerUrl =
config.getProperty(LdapAuthenticationHandlerImpl.PROVIDER_URL)
+this.ldapDomain =
config.getProperty(LdapAuthenticationHandlerImpl.LDAP_BIND_DOMAIN)
+this.enableStartTls =
config.getProperty(LdapAuthenticationHandlerImpl.ENABLE_START_TLS,
+ "false").toBoolean
+require(this.providerUrl != null, "The LDAP URI can not be null")
+
+if (enableStartTls) {
+ require(!this.providerUrl.toLowerCase.startsWith("ldaps"),
+"Can not use ldaps and StartTLS option at the same time")
+}
+ }
+
+ def destroy(): Unit = { }
+
+ @throws[IOException]
+ @throws[AuthenticationException]
+ def managementOperation(token: AuthenticationToken,
+ request: HttpServletRequest,
+ response: HttpServletResponse): Boolean = true
Review comment:
Here the style should also be changed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage process message to the end user using thriftserver
jerryshao commented on a change in pull request #238: [LIVY-689] Deliver stage
process message to the end user using thriftserver
URL: https://github.com/apache/incubator-livy/pull/238#discussion_r333806025
##
File path: rsc/src/main/java/org/apache/livy/rsc/driver/JobWrapper.java
##
@@ -60,6 +70,14 @@ public Void call() throws Exception {
}
}
+timer.schedule(new TimerTask() {
+@Override
+public void run() {
+driver.handleProcessMessage(jobId);
+}
+}, firstDelayMSec, updatePeriodMSec);
Review comment:
Two space indent here.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server.
codecov-io edited a comment on issue #231: [LIVY-356][SERVER]Add LDAP authentication for livy-server. URL: https://github.com/apache/incubator-livy/pull/231#issuecomment-534468593 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=h1) Report > Merging [#231](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/0804c8ea8ece67d01ababec616c9ad8e3b15dc9f?src=pr=desc) will **decrease** coverage by `0.39%`. > The diff coverage is `50.8%`. [](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #231 +/- ## === - Coverage 68.45% 68.06% -0.4% - Complexity 927 939 +12 === Files 100 101 +1 Lines 5729 5853+124 Branches870 886 +16 === + Hits 3922 3984 +62 - Misses 1247 1297 +50 - Partials560 572 +12 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.33% <0%> (-2.14%)` | `11 <0> (ø)` | | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.97% <100%> (+0.1%)` | `21 <0> (ø)` | :arrow_down: | | [...vy/server/auth/LdapAuthenticationHandlerImpl.scala](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvYXV0aC9MZGFwQXV0aGVudGljYXRpb25IYW5kbGVySW1wbC5zY2FsYQ==) | `56.31% <56.31%> (ø)` | `13 <13> (?)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/231/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `79.91% <0%> (-0.84%)` | `45% <0%> (-1%)` | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=footer). Last update [0804c8e...62d8fa0](https://codecov.io/gh/apache/incubator-livy/pull/231?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
