Add Support for some kubernetes client settings #469

duyanghao · 2017-08-29T10:25:03Z

What changes were proposed in this pull request?

Add Support for spark.kubernetes.client.watch.reconnectInterval(which is relevant to KUBERNETES_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.)
Add Support for spark.kubernetes.client.watch.reconnectLimit(which is relevant to KUBERNETES_WATCH_RECONNECT_LIMIT_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.)
Add Support for spark.kubernetes.client.connection.timeout(which is relevant to KUBERNETES_CONNECTION_TIMEOUT_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.)
Add Support for spark.kubernetes.client.request.timeout(which is relevant to KUBERNETES_REQUEST_TIMEOUT_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.)

How was this patch tested?

start submit with following pars:

--conf spark.kubernetes.client.connection.timeout=30000 \
--conf spark.kubernetes.client.request.timeout=30000 \
--conf spark.kubernetes.client.watch.reconnectInterval=5000 \
--conf spark.kubernetes.client.watch.reconnectLimit=5 \

Tests on my cluster show that above settings do work.

liyinan926 · 2017-08-29T18:31:15Z

...es/core/src/main/scala/org/apache/spark/deploy/kubernetes/SparkKubernetesClientFactory.scala

@@ -81,6 +81,30 @@ private[spark] object SparkKubernetesClientFactory {
        }.withOption(namespace) {
          (ns, configBuilder) => configBuilder.withNamespace(ns)
        }.build()
+    val conn_timeout_properties = sparkConf


Camel case please for all the new vals.

liyinan926 · 2017-08-29T18:32:15Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

@@ -545,4 +545,36 @@ package object config extends Logging {
      resolvedURL
    }
  }
+
+  private[spark] val KUBERNETES_CLIENT_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY =
+    ConfigBuilder("spark.kubernetes.client.watch.reconnectInterval")


The configuration properties should include time unit in the property names, e.g., reconnectInternalInMs.

liyinan926 · 2017-08-29T18:33:33Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

+
+  private[spark] val KUBERNETES_CLIENT_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY =
+    ConfigBuilder("spark.kubernetes.client.watch.reconnectInterval")
+      .doc("spark kubernetes client watch reconnectInterval," +


All of our config properties docs start with a Capital letter.

liyinan926 · 2017-08-29T18:35:14Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

@@ -545,4 +545,36 @@ package object config extends Logging {
      resolvedURL
    }
  }
+
+  private[spark] val KUBERNETES_CLIENT_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY =
+    ConfigBuilder("spark.kubernetes.client.watch.reconnectInterval")


You might want to define a command prefix for the keys.

mccheah · 2017-08-29T20:34:53Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

+      .doc("spark kubernetes client watch reconnectInterval," +
+        "which is relevant to KUBERNETES_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY" +
+        " in io.fabric8.kubernetes.client.Config.")
+      .stringConf


Use intConf with default values everywhere.

Actually timeConfmight make some sense in some of these cases. I think these should have default values that are specified in the config entries and not in the main logic.

mccheah · 2017-08-29T20:36:52Z

Document all new properties in docs/running-on-kubernetes.md as well as in https://github.com/apache-spark-on-k8s/userdocs after this merges.

duyanghao · 2017-08-30T04:47:47Z

@mccheah

Document all new properties in docs/running-on-kubernetes.md as well as in https://github.com/apache-spark-on-k8s/userdocs after this merges.

Done for docs/running-on-kubernetes.md

liyinan926 · 2017-08-30T23:06:59Z

Please rebase against the latest branch-2.2-kubernetes.

mccheah · 2017-08-30T23:08:48Z

docs/running-on-kubernetes.md

+  <td><code>1s</code></td>
+  <td>
+    Spark kubernetes client watch reconnectInterval, which is relevant to
+    <code>KUBERNETES_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY</code> in io.fabric8.kubernetes.client.Config.


Use semantic meanings here as opposed to meanings specific to the code implementation. For example, this documentation becomes incorrect if we end up not using the Fabric8 Kubernetes client. The other settings provide some examples that this can be based on.

mccheah · 2017-08-30T23:09:58Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

+
+  private[spark] val KUBERNETES_CLIENT_WATCH_RECONNECT_LIMIT_SYSTEM_PROPERTY =
+    ConfigBuilder("spark.kubernetes.client.watch.reconnectLimit")
+      .doc("Spark kubernetes client watch reconnectLimit," +


Also use semantic meanings in these docs, as opposed to implementation specific. "Limit of times where connections can be attempted for watching resources" or something.

duyanghao · 2017-09-01T01:24:45Z

@mccheah @liyinan926

Please rebase against the latest branch-2.2-kubernetes.

Done

liyinan926 · 2017-09-01T02:53:37Z

docs/running-on-kubernetes.md

+  <td><code>spark.kubernetes.client.watch.reconnectLimit</code></td>
+  <td><code>-1</code></td>
+  <td>
+    Limit of times where connections can be attempted for kubernetes client request.


Delete where. And change request to requests.

liyinan926 · 2017-09-01T02:54:11Z

Minor comment. Also it seems this is still out-of-date with the base branch.

duyanghao

@liyinan926 rebase Done.

duyanghao · 2017-09-01T04:42:25Z

docs/running-on-kubernetes.md

+  <td><code>spark.kubernetes.client.watch.reconnectLimit</code></td>
+  <td><code>-1</code></td>
+  <td>
+    Limit of times where connections can be attempted for kubernetes client request.


Add Support for `spark.kubernetes.client.watch.reconnectInterval`(which is relevant to KUBERNETES_WATCH_RECONNECT_INTERVAL_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.) Add Support for `spark.kubernetes.client.watch.reconnectLimit`(which is relevant to KUBERNETES_WATCH_RECONNECT_LIMIT_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.) Add Support for `spark.kubernetes.client.connection.timeout`(which is relevant to KUBERNETES_CONNECTION_TIMEOUT_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.) Add Support for `spark.kubernetes.client.request.timeout`(which is relevant to KUBERNETES_REQUEST_TIMEOUT_SYSTEM_PROPERTY in io.fabric8.kubernetes.client.Config.) Signed-off-by: duyanghao <1294057873@qq.com>

liyinan926 · 2017-09-01T15:14:34Z

LGTM.

mccheah · 2017-09-06T00:10:15Z

After the build passes following the updated branch we can merge this.

Signed-off-by: duyanghao <1294057873@qq.com>

… broadcast object (apache-spark-on-k8s#469) … broadcast object ## What changes were proposed in this pull request? This PR changes the broadcast object in TorrentBroadcast from a strong reference to a weak reference. This allows it to be garbage collected even if the Dataset is held in memory. This is ok, because the broadcast object can always be re-read. ## How was this patch tested? Tested in Spark shell by taking a heap dump, full repro steps listed in https://issues.apache.org/jira/browse/SPARK-25998. Closes apache#22995 from bkrieger/bk/torrent-broadcast-weak. Authored-by: Brandon Krieger <bkrieger@palantir.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

duyanghao mentioned this pull request Aug 29, 2017

java.net.SocketTimeoutException: timeout #465

Open

duyanghao force-pushed the branch-2.2-kubernetes branch 2 times, most recently from 854192b to 746dea7 Compare August 29, 2017 11:58

liyinan926 reviewed Aug 29, 2017

View reviewed changes

mccheah reviewed Aug 29, 2017

View reviewed changes

duyanghao force-pushed the branch-2.2-kubernetes branch from 746dea7 to 5038d68 Compare August 30, 2017 04:45

duyanghao force-pushed the branch-2.2-kubernetes branch from 5038d68 to d8ba177 Compare August 30, 2017 06:28

mccheah reviewed Aug 30, 2017

View reviewed changes

duyanghao force-pushed the branch-2.2-kubernetes branch from 55eb126 to 7ad8238 Compare August 31, 2017 03:43

liyinan926 reviewed Sep 1, 2017

View reviewed changes

duyanghao commented Sep 1, 2017

View reviewed changes

duyanghao force-pushed the branch-2.2-kubernetes branch from 17c52d5 to 7f06745 Compare September 1, 2017 06:46

Merge branch 'branch-2.2-kubernetes' into branch-2.2-kubernetes

598fb25

duyanghao added 2 commits March 14, 2018 14:34

rebase

5a07371

Signed-off-by: duyanghao <1294057873@qq.com>

Add Recovery Logic for Failed Pod

81c4586

Signed-off-by: duyanghao <1294057873@qq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for some kubernetes client settings #469

Add Support for some kubernetes client settings #469

duyanghao commented Aug 29, 2017

liyinan926 Aug 29, 2017

duyanghao Aug 30, 2017

liyinan926 Aug 29, 2017

duyanghao Aug 30, 2017

liyinan926 Aug 29, 2017

duyanghao Aug 30, 2017

liyinan926 Aug 29, 2017

duyanghao Aug 30, 2017

mccheah Aug 29, 2017

mccheah Aug 29, 2017

duyanghao Aug 30, 2017

mccheah commented Aug 29, 2017

duyanghao commented Aug 30, 2017 •

edited

Loading

liyinan926 commented Aug 30, 2017

mccheah Aug 30, 2017

duyanghao Aug 31, 2017

mccheah Aug 30, 2017

duyanghao Aug 31, 2017

duyanghao commented Sep 1, 2017 •

edited

Loading

liyinan926 Sep 1, 2017 •

edited

Loading

duyanghao Sep 1, 2017

liyinan926 commented Sep 1, 2017

duyanghao left a comment

duyanghao Sep 1, 2017

liyinan926 commented Sep 1, 2017

mccheah commented Sep 6, 2017

Add Support for some kubernetes client settings #469

Are you sure you want to change the base?

Add Support for some kubernetes client settings #469

Conversation

duyanghao commented Aug 29, 2017

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccheah commented Aug 29, 2017

duyanghao commented Aug 30, 2017 • edited Loading

liyinan926 commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duyanghao commented Sep 1, 2017 • edited Loading

liyinan926 Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyinan926 commented Sep 1, 2017

duyanghao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyinan926 commented Sep 1, 2017

mccheah commented Sep 6, 2017

duyanghao commented Aug 30, 2017 •

edited

Loading

duyanghao commented Sep 1, 2017 •

edited

Loading

liyinan926 Sep 1, 2017 •

edited

Loading