You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@livy.apache.org by GitBox <gi...@apache.org> on 2019/12/20 02:49:42 UTC
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve two targets:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
2. ZooKeeperManager which contains the utilities of zookeeper should be a single instance.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361269521
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ if (!zkUrl.isEmpty) {
+ zkUrl
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ }
+ }
+
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = {
+ val retryConf = livyConf.get(LivyConf.ZK_RETRY_POLICY)
+ if (!retryConf.isEmpty) {
+ retryConf
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ }
+ }
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ Runtime.getRuntime.addShutdownHook(new Thread(new Runnable {
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r363198595
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL))).
+ map(_.trim).orNull
+
+ require(zkAddress != null && !zkAddress.isEmpty,
+ s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY))).
+ map(_.trim).orNull
+
+ require(retryValue != null && !retryValue.isEmpty,
+ s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error: ${message}.", e)
+ }
+ })
+
+ def start(): Unit = {
+ curatorClient.start()
+ }
+
+ def stop(): Unit = {
+ curatorClient.close()
+ }
+
+ // TODO Make sure ZK path has proper secure permissions so that other users cannot read its
+ // contents.
+
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360752386
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
Review comment:
So it may be a little confusing that the key name contains recovery? Looks like it's for recovery.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361278263
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
Review comment:
This is a legacy code, but I think here `System.exit(1)` is too overkill, maybe we should have a better way to handle this issue.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360797615
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
Review comment:
If use it as a normal class object, I need to pass the object of ZooKeeperManager as a param to everywhere use it, and a lot of code has to 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r363149183
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL))).
+ map(_.trim).orNull
+
+ require(zkAddress != null && !zkAddress.isEmpty,
+ s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY))).
+ map(_.trim).orNull
+
+ require(retryValue != null && !retryValue.isEmpty,
+ s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error: ${message}.", e)
+ }
+ })
+
+ def start(): Unit = {
+ curatorClient.start()
+ }
+
+ def stop(): Unit = {
+ curatorClient.close()
+ }
+
+ // TODO Make sure ZK path has proper secure permissions so that other users cannot read its
+ // contents.
+
Review comment:
Please remove this blank 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361377060
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
+ }
+ })
+
+ def startCuratorClient(): Unit = {
+ curatorClient.start()
+ }
+
+ def stopCuratorClient(): Unit = {
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361241492
##########
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##########
@@ -196,9 +200,33 @@ object LivyConf {
* For filesystem state store, the path of the state store directory. Please don't use a
* filesystem that doesn't support atomic rename (e.g. S3). e.g. file:///tmp/livy or hdfs:///.
* For zookeeper, the address to the Zookeeper servers. e.g. host1:port1,host2:port2
+ * If livy.server.recovery.state-store is zookeeper, this config is for back-compatibility,
+ * so if both this config and livy.server.zookeeper.url exist,
+ * livy uses livy.server.zookeeper.url first.
*/
val RECOVERY_STATE_STORE_URL = Entry("livy.server.recovery.state-store.url", "")
Review comment:
Also for this.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361241470
##########
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##########
@@ -185,6 +185,10 @@ object LivyConf {
* configure the state store.
*/
val RECOVERY_MODE = Entry("livy.server.recovery.mode", "off")
+
+ // Zookeeper address used for HA and state store. e.g. host1:port1, host2:port2
+ val ZOOKEEPER_URL = Entry("livy.server.zookeeper.url", "")
Review comment:
The default value of this can be null, not empty string.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361277716
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
+ }
+ })
+
+ def startCuratorClient(): Unit = {
+ curatorClient.start()
+ }
+
+ def stopCuratorClient(): Unit = {
Review comment:
Also here change to `stop`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361269509
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `70.83%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.05% -0.13%
- Complexity 945 950 +5
============================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 898 +2
============================================
+ Hits 4022 4030 +8
- Misses 1303 1309 +6
- Partials 574 583 +9
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `68.29% <68.29%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.66% <0%> (-1.26%)` | `43% <0%> (-2%)` | |
| ... and [1 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...eafbca0](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `70.83%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.05% -0.13%
- Complexity 945 950 +5
============================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 898 +2
============================================
+ Hits 4022 4030 +8
- Misses 1303 1309 +6
- Partials 574 583 +9
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `68.29% <68.29%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.66% <0%> (-1.26%)` | `43% <0%> (-2%)` | |
| ... and [1 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...eafbca0](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361277616
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
Review comment:
`Option(xxx).orElse(Option(yyy)).map(_.trim).orNull`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `71.01%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.13% -0.06%
- Complexity 945 951 +6
============================================
Files 102 103 +1
Lines 5899 5921 +22
Branches 896 898 +2
============================================
+ Hits 4022 4034 +12
- Misses 1303 1309 +6
- Partials 574 578 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.88% <50%> (ø)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `67.44% <67.44%> (ø)` | `16 <16> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `80.75% <0%> (+0.83%)` | `45% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...fadf767](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360752301
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
+ }
+}
+
+class ZooKeeperManager private(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ import ZooKeeperManager._
+
+ private val zkAddress = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
Review comment:
So it may be a little confusing that the key name contains `recovery`? Looks like it's for recovery.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361377040
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360336611
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
Review comment:
ZK_RETRY_CONF is a try policy of client connecting to ZooKeeper, and I think it's better to keep it in ZooKeeperManager. And the name i.e. `livy.server.recovery.zk-state-store.retry-policy` should not be changed, in case user config it in livy.conf.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361377017
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361277910
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
Review comment:
Also 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360753325
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
Review comment:
This is an init method. Init method has side-effect and changes the context. But it usually doesn't return a value. For example, see [here](https://github.com/apache/incubator-livy/blob/master/server/src/main/scala/org/apache/livy/server/recovery/StateStore.scala#L81), [here](https://github.com/apache/incubator-livy/blob/master/server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala#L40) and [here](https://github.com/apache/incubator-livy/blob/master/server/src/main/scala/org/apache/livy/server/AccessFilter.scala#L25)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360800236
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
Review comment:
Okay, I will change it to a normal class object.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `70.83%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.06% -0.12%
- Complexity 945 952 +7
============================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 898 +2
============================================
+ Hits 4022 4031 +9
- Misses 1303 1311 +8
- Partials 574 580 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `68.29% <68.29%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...c/src/main/java/org/apache/livy/rsc/RSCClient.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9SU0NDbGllbnQuamF2YQ==) | `72.28% <0%> (-1.21%)` | `20% <0%> (ø)` | |
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0%> (ø)` | `2% <0%> (ø)` | :arrow_down: |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `79.91% <0%> (ø)` | `45% <0%> (ø)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...c63894f](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360752301
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
+ }
+}
+
+class ZooKeeperManager private(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ import ZooKeeperManager._
+
+ private val zkAddress = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
Review comment:
So it may be a little confusing that the key name contains `recovery`? Looks like it's for recovery.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.1%`.
> The diff coverage is `69.86%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.07% -0.11%
- Complexity 945 952 +7
============================================
Files 102 103 +1
Lines 5899 5923 +24
Branches 896 898 +2
============================================
+ Hits 4022 4032 +10
- Misses 1303 1311 +8
- Partials 574 580 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66.66% <66.66%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0%> (ø)` | `2% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...704e46a](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361269557
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ if (!zkUrl.isEmpty) {
+ zkUrl
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ }
+ }
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.1%`.
> The diff coverage is `69.86%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.07% -0.11%
- Complexity 945 952 +7
============================================
Files 102 103 +1
Lines 5899 5923 +24
Branches 896 898 +2
============================================
+ Hits 4022 4032 +10
- Misses 1303 1311 +8
- Partials 574 580 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66.66% <66.66%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0%> (ø)` | `2% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...704e46a](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360737148
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
Review comment:
It's not just a state-store configuration, it's a configuration about where to create all zookeeper node, so it's suitable to put it 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361377143
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/FileSystemStateStore.scala
##########
@@ -44,7 +44,7 @@ class FileSystemStateStore(
private val fsUri = {
val fsPath = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
- require(!fsPath.isEmpty, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
+ require(fsPath != null, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `68%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.06% -0.12%
- Complexity 945 952 +7
============================================
Files 102 104 +2
Lines 5899 5925 +26
Branches 896 898 +2
============================================
+ Hits 4022 4033 +11
- Misses 1303 1311 +8
- Partials 574 581 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...a/org/apache/livy/utils/LivyUncaughtException.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9MaXZ5VW5jYXVnaHRFeGNlcHRpb24uamF2YQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66.66% <66.66%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...ff490cf](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360752446
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
Review comment:
I think the key name is confusing
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360799913
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
Review comment:
That's fine, and that is what we do currently for other modules. I don't think there're many classes which need to 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361377082
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
+ }
+ })
+
+ def startCuratorClient(): Unit = {
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `70.83%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.05% -0.13%
- Complexity 945 950 +5
============================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 898 +2
============================================
+ Hits 4022 4030 +8
- Misses 1303 1309 +6
- Partials 574 583 +9
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `68.29% <68.29%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.66% <0%> (-1.26%)` | `43% <0%> (-2%)` | |
| ... and [1 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...eafbca0](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360738619
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
Review comment:
why return nothing ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `72.22%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
===========================================
- Coverage 68.18% 68.08% -0.1%
- Complexity 945 950 +5
===========================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 896
===========================================
+ Hits 4022 4032 +10
- Misses 1303 1311 +8
- Partials 574 579 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `62.5% <100%> (ø)` | `11 <0> (ø)` | :arrow_down: |
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `69.04% <69.04%> (ø)` | `15 <15> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...7f85e85](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360737148
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
Review comment:
It's not just a state-store configuration, it's a configuration about where to create all zookeeper node such as node for service discovery and distributed lock, so it's suitable to put it 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on issue #267: [LIVY-732][SERVER]
A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-568744846
+1
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360753325
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
Review comment:
This is an init method. Init method has side-effect and changes the context. But it usually doesn't return a value. For example, see (here)[https://github.com/apache/incubator-livy/blob/master/server/src/main/scala/org/apache/livy/server/recovery/StateStore.scala#L81], (here)[https://github.com/apache/incubator-livy/blob/master/server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala#L40] and [here](https://github.com/apache/incubator-livy/blob/master/server/src/main/scala/org/apache/livy/server/AccessFilter.scala#L25)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360753564
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
+ }
+}
+
+class ZooKeeperManager private(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ import ZooKeeperManager._
+
+ private val zkAddress = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
Review comment:
So if user doesn't configure it, zkAddress is an empty string?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361242089
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ if (!zkUrl.isEmpty) {
+ zkUrl
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ }
+ }
+
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = {
+ val retryConf = livyConf.get(LivyConf.ZK_RETRY_POLICY)
+ if (!retryConf.isEmpty) {
+ retryConf
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ }
+ }
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ Runtime.getRuntime.addShutdownHook(new Thread(new Runnable {
Review comment:
the stop of this class could be added to `LivyServer`'s ShutdownHook.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.1%`.
> The diff coverage is `69.86%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.07% -0.11%
- Complexity 945 952 +7
============================================
Files 102 103 +1
Lines 5899 5923 +24
Branches 896 898 +2
============================================
+ Hits 4022 4032 +10
- Misses 1303 1311 +8
- Partials 574 580 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66.66% <66.66%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0%> (ø)` | `2% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...704e46a](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `70.83%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.06% -0.12%
- Complexity 945 952 +7
============================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 898 +2
============================================
+ Hits 4022 4031 +9
- Misses 1303 1311 +8
- Partials 574 580 +6
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `68.29% <68.29%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...c/src/main/java/org/apache/livy/rsc/RSCClient.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9SU0NDbGllbnQuamF2YQ==) | `72.28% <0%> (-1.21%)` | `20% <0%> (ø)` | |
| [.../scala/org/apache/livy/sessions/SessionState.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-Y29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvc2Vzc2lvbnMvU2Vzc2lvblN0YXRlLnNjYWxh) | `61.11% <0%> (ø)` | `2% <0%> (ø)` | :arrow_down: |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `79.91% <0%> (ø)` | `45% <0%> (ø)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...c63894f](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `68%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.06% -0.12%
- Complexity 945 952 +7
============================================
Files 102 104 +2
Lines 5899 5925 +26
Branches 896 898 +2
============================================
+ Hits 4022 4033 +11
- Misses 1303 1311 +8
- Partials 574 581 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...a/org/apache/livy/utils/LivyUncaughtException.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9MaXZ5VW5jYXVnaHRFeGNlcHRpb24uamF2YQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66.66% <66.66%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...ff490cf](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve two targets:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
2. ZooKeeperManager which contains the utilities of zookeeper should be a single instance.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361377120
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361269534
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ if (!zkUrl.isEmpty) {
+ zkUrl
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ }
+ }
+
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = {
+ val retryConf = livyConf.get(LivyConf.ZK_RETRY_POLICY)
+ if (!retryConf.isEmpty) {
+ retryConf
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ }
+ }
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ Runtime.getRuntime.addShutdownHook(new Thread(new Runnable {
+ override def run(): Unit = {
+ curatorClient.close()
+ }
+ }))
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
+ }
+ })
+ curatorClient.start()
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yiheng commented on a change in pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
yiheng commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360753995
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
Review comment:
Then I suggest you use another key. In zk-state-store, it first looks at the `livy.server.recovery.zk-state-store.key-prefix`, then look at the new key. What do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361269581
##########
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##########
@@ -185,6 +185,10 @@ object LivyConf {
* configure the state store.
*/
val RECOVERY_MODE = Entry("livy.server.recovery.mode", "off")
+
+ // Zookeeper address used for HA and state store. e.g. host1:port1, host2:port2
+ val ZOOKEEPER_URL = Entry("livy.server.zookeeper.url", "")
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361241600
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ if (!zkUrl.isEmpty) {
+ zkUrl
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ }
+ }
Review comment:
If the default value is null, then this can be simplified to
```
Option(xxx).getOrElse(xxxx)
require(xxxx != null)
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361277453
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/FileSystemStateStore.scala
##########
@@ -44,7 +44,7 @@ class FileSystemStateStore(
private val fsUri = {
val fsPath = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
- require(!fsPath.isEmpty, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
+ require(fsPath != null, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
Review comment:
You should also check if this parameter is empty or not.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361269576
##########
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##########
@@ -196,9 +200,33 @@ object LivyConf {
* For filesystem state store, the path of the state store directory. Please don't use a
* filesystem that doesn't support atomic rename (e.g. S3). e.g. file:///tmp/livy or hdfs:///.
* For zookeeper, the address to the Zookeeper servers. e.g. host1:port1,host2:port2
+ * If livy.server.recovery.state-store is zookeeper, this config is for back-compatibility,
+ * so if both this config and livy.server.zookeeper.url exist,
+ * livy uses livy.server.zookeeper.url first.
*/
val RECOVERY_STATE_STORE_URL = Entry("livy.server.recovery.state-store.url", "")
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360333989
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
+ }
+}
+
+class ZooKeeperManager private(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ import ZooKeeperManager._
+
+ private val zkAddress = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.RECOVERY_STATE_STORE_URL.key}.")
Review comment:
zkAddress is a String.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360795968
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
+ val ZK_RETRY_CONF = Entry("livy.server.recovery.zk-state-store.retry-policy", "5,100")
+ val DUPLICATE_CREATE_EXCEPTION = "ZooKeeperManager single instance has already been created"
+
+ @volatile private var zkManager: ZooKeeperManager = _
+
+ def apply(livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None):
+ ZooKeeperManager = synchronized {
+ if (zkManager == null) {
+ zkManager = new ZooKeeperManager(livyConf, mockCuratorClient)
+ } else {
+ throw new IllegalAccessException(DUPLICATE_CREATE_EXCEPTION)
+ }
+
+ zkManager
+ }
+
+ def get(): ZooKeeperManager = zkManager
+
+ // for test
+ private[recovery] def reset(): Unit = {
+ zkManager = null
Review comment:
I personally don't like singleton here, it is hard for testing, and that's why you need to add this method here. Why don't we change to a normal class object?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang closed pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r363198595
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL))).
+ map(_.trim).orNull
+
+ require(zkAddress != null && !zkAddress.isEmpty,
+ s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY))).
+ map(_.trim).orNull
+
+ require(retryValue != null && !retryValue.isEmpty,
+ s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error: ${message}.", e)
+ }
+ })
+
+ def start(): Unit = {
+ curatorClient.start()
+ }
+
+ def stop(): Unit = {
+ curatorClient.close()
+ }
+
+ // TODO Make sure ZK path has proper secure permissions so that other users cannot read its
+ // contents.
+
Review comment:
updated
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361242012
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
+ if (!zkUrl.isEmpty) {
+ zkUrl
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ }
+ }
+
+ require(!zkAddress.isEmpty, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = {
+ val retryConf = livyConf.get(LivyConf.ZK_RETRY_POLICY)
+ if (!retryConf.isEmpty) {
+ retryConf
+ } else {
+ // for back-compatibility
+ livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ }
+ }
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ Runtime.getRuntime.addShutdownHook(new Thread(new Runnable {
+ override def run(): Unit = {
+ curatorClient.close()
+ }
+ }))
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
+ }
+ })
+ curatorClient.start()
Review comment:
I think we should declare start() and stop() method and call it out of this class (maybe `LiveServer` I think).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361277616
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
Review comment:
`Option(xxx).orElse(yyy).map(_.trim).orNull`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361048063
##########
File path: conf/livy.conf.template
##########
@@ -116,7 +118,7 @@
# For filesystem state store, the path of the state store directory. Please don't use a filesystem
# that doesn't support atomic rename (e.g. S3). e.g. file:///tmp/livy or hdfs:///.
-# For zookeeper, the address to the Zookeeper servers. e.g. host1:port1,host2:port2
+# For zookeeper, please set livy.server.zookeeper.url
Review comment:
What if state-store is configured to use filesystem?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `71.01%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.13% -0.06%
- Complexity 945 951 +6
============================================
Files 102 103 +1
Lines 5899 5921 +22
Branches 896 898 +2
============================================
+ Hits 4022 4034 +12
- Misses 1303 1309 +6
- Partials 574 578 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.88% <50%> (ø)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `67.44% <67.44%> (ø)` | `16 <16> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `80.75% <0%> (+0.83%)` | `45% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...fadf767](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `70.83%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.05% -0.13%
- Complexity 945 950 +5
============================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 898 +2
============================================
+ Hits 4022 4030 +8
- Misses 1303 1309 +6
- Partials 574 583 +9
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `68.29% <68.29%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.66% <0%> (-1.26%)` | `43% <0%> (-2%)` | |
| ... and [1 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...eafbca0](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r363149382
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,118 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL))).
+ map(_.trim).orNull
+
+ require(zkAddress != null && !zkAddress.isEmpty,
+ s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).
+ orElse(Option(livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY))).
+ map(_.trim).orNull
+
+ require(retryValue != null && !retryValue.isEmpty,
+ s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error: ${message}.", e)
Review comment:
I would suggest to add code to throw uncaught exception, so that upstream could handle it, or just throw exception and finish the program.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r360752682
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,137 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.LivyConf.Entry
+import org.apache.livy.Logging
+
+object ZooKeeperManager {
+ val ZK_KEY_PREFIX_CONF = Entry("livy.server.recovery.zk-state-store.key-prefix", "livy")
Review comment:
Yes, but for back-compatibility, it's better to keep the name unchanged.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] runzhiwang opened a new pull request #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
runzhiwang opened a new pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267
## What changes were proposed in this pull request?
Currently, the utilities of zookeeper mixed with ZooKeeperStateStore. To use the utility of zookeeper, the instance of ZooKeeperStateStore has to be created , which looks weird.
This PR aims to achieve the follow target:
1. Extract the utilities of zookeeper from ZooKeeperStateStore to support such as distributed lock, service discovery and so on.
## How was this patch tested?
Existed UT and 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361242504
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = {
+ val zkUrl = livyConf.get(LivyConf.ZOOKEEPER_URL)
Review comment:
We should also trim the string to remove unnecessary ws.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `72.22%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
===========================================
- Coverage 68.18% 68.08% -0.1%
- Complexity 945 950 +5
===========================================
Files 102 103 +1
Lines 5899 5922 +23
Branches 896 896
===========================================
+ Hits 4022 4032 +10
- Misses 1303 1311 +8
- Partials 574 579 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `62.5% <100%> (ø)` | `11 <0> (ø)` | :arrow_down: |
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.03% <50%> (+0.15%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `69.04% <69.04%> (ø)` | `15 <15> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...7f85e85](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `71.01%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.13% -0.06%
- Complexity 945 951 +6
============================================
Files 102 103 +1
Lines 5899 5921 +22
Branches 896 898 +2
============================================
+ Hits 4022 4034 +12
- Misses 1303 1309 +6
- Partials 574 578 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `32.88% <50%> (ø)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `67.44% <67.44%> (ø)` | `16 <16> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `80.75% <0%> (+0.83%)` | `45% <0%> (ø)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...fadf767](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #267:
[LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#issuecomment-567787396
# [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=h1) Report
> Merging [#267](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/25542e4e78b39a3c9b9426a70a92ca7c183daea3?src=pr&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `68%`.
[](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #267 +/- ##
============================================
- Coverage 68.18% 68.06% -0.12%
- Complexity 945 952 +7
============================================
Files 102 104 +2
Lines 5899 5925 +26
Branches 896 898 +2
============================================
+ Hits 4022 4033 +11
- Misses 1303 1311 +8
- Partials 574 581 +7
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...a/org/apache/livy/utils/LivyUncaughtException.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9MaXZ5VW5jYXVnaHRFeGNlcHRpb24uamF2YQ==) | `0% <0%> (ø)` | `0 <0> (?)` | |
| [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.11%)` | `21 <0> (ø)` | :arrow_down: |
| [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `100% <100%> (+22.72%)` | `8 <8> (-9)` | :arrow_down: |
| [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `72.41% <42.85%> (-10.92%)` | `1 <0> (ø)` | |
| [...he/livy/server/recovery/FileSystemStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvRmlsZVN5c3RlbVN0YXRlU3RvcmUuc2NhbGE=) | `61.22% <50%> (-1.28%)` | `11 <0> (ø)` | |
| [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `33.48% <50%> (+0.6%)` | `11 <0> (ø)` | :arrow_down: |
| [...apache/livy/server/recovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyTWFuYWdlci5zY2FsYQ==) | `66.66% <66.66%> (ø)` | `17 <17> (?)` | |
| [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | |
| [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | |
| ... and [2 more](https://codecov.io/gh/apache/incubator-livy/pull/267/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=footer). Last update [25542e4...ff490cf](https://codecov.io/gh/apache/incubator-livy/pull/267?src=pr&el=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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on a change in pull request
#267: [LIVY-732][SERVER] A common zookeeper wrapper utility
Posted by GitBox <gi...@apache.org>.
jerryshao commented on a change in pull request #267: [LIVY-732][SERVER] A common zookeeper wrapper utility
URL: https://github.com/apache/incubator-livy/pull/267#discussion_r361277694
##########
File path: server/src/main/scala/org/apache/livy/server/recovery/ZooKeeperManager.scala
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.recovery
+
+import scala.collection.JavaConverters._
+import scala.reflect.ClassTag
+
+import org.apache.curator.framework.api.UnhandledErrorListener
+import org.apache.curator.framework.CuratorFramework
+import org.apache.curator.framework.CuratorFrameworkFactory
+import org.apache.curator.retry.RetryNTimes
+import org.apache.zookeeper.KeeperException.NoNodeException
+
+import org.apache.livy.LivyConf
+import org.apache.livy.Logging
+
+class ZooKeeperManager(
+ livyConf: LivyConf,
+ mockCuratorClient: Option[CuratorFramework] = None)
+ extends JsonMapper with Logging {
+
+ def this(livyConf: LivyConf) {
+ this(livyConf, None)
+ }
+
+ private val zkAddress = Option(livyConf.get(LivyConf.ZOOKEEPER_URL)).getOrElse {
+ // for back-compatibility
+ val url = livyConf.get(LivyConf.RECOVERY_STATE_STORE_URL)
+ require(url != null, s"Please config ${LivyConf.ZOOKEEPER_URL.key}.")
+ url
+ }.trim
+
+ private val retryValue = Option(livyConf.get(LivyConf.ZK_RETRY_POLICY)).getOrElse {
+ // for back-compatibility
+ val policy = livyConf.get(LivyConf.RECOVERY_ZK_STATE_STORE_RETRY_POLICY)
+ require(policy != null, s"Please config ${LivyConf.ZK_RETRY_POLICY.key}.")
+ policy
+ }.trim
+
+ // a regex to match patterns like "m, n" where m and n both are integer values
+ private val retryPattern = """\s*(\d+)\s*,\s*(\d+)\s*""".r
+ private[recovery] val retryPolicy = retryValue match {
+ case retryPattern(n, sleepMs) => new RetryNTimes(n.toInt, sleepMs.toInt)
+ case _ => throw new IllegalArgumentException(
+ s"contains bad value: $retryValue. " +
+ "Correct format is <max retry count>,<sleep ms between retry>. e.g. 5,100")
+ }
+
+ private val curatorClient = mockCuratorClient.getOrElse {
+ CuratorFrameworkFactory.newClient(zkAddress, retryPolicy)
+ }
+
+ curatorClient.getUnhandledErrorListenable().addListener(new UnhandledErrorListener {
+ def unhandledError(message: String, e: Throwable): Unit = {
+ error(s"Fatal Zookeeper error. Shutting down Livy server.")
+ System.exit(1)
+ }
+ })
+
+ def startCuratorClient(): Unit = {
Review comment:
Simplify to `start()`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services