Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

Add Support for some kubernetes client settings - #469

Open
duyanghao wants to merge 4 commits into
apache-spark-on-k8s:branch-2.2-kubernetesfrom
duyanghao:branch-2.2-kubernetes
Open

Add Support for some kubernetes client settings#469
duyanghao wants to merge 4 commits into
apache-spark-on-k8s:branch-2.2-kubernetesfrom
duyanghao:branch-2.2-kubernetes

Conversation

@duyanghao

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Camel case please for all the new vals.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use intConf with default values everywhere.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mccheah

mccheah commented Aug 29, 2017

Copy link
Copy Markdown

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
duyanghao force-pushed the branch-2.2-kubernetes branch from 746dea7 to 5038d68 Compare August 30, 2017 04:45
@duyanghao

duyanghao commented Aug 30, 2017

Copy link
Copy Markdown
Author

@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

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

Copy link
Copy Markdown
Member

Please rebase against the latest branch-2.2-kubernetes.

Comment thread docs/running-on-kubernetes.md Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

duyanghao commented Sep 1, 2017

Copy link
Copy Markdown
Author

@mccheah @liyinan926

Please rebase against the latest branch-2.2-kubernetes.

Done

Comment thread docs/running-on-kubernetes.md Outdated

@liyinan926 liyinan926 Sep 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete where. And change request to requests.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@liyinan926

Copy link
Copy Markdown
Member

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

@duyanghao duyanghao left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liyinan926 rebase Done.

Comment thread docs/running-on-kubernetes.md Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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>
@duyanghao
duyanghao force-pushed the branch-2.2-kubernetes branch from 17c52d5 to 7f06745 Compare September 1, 2017 06:46
@liyinan926

Copy link
Copy Markdown
Member

LGTM.

@mccheah

mccheah commented Sep 6, 2017

Copy link
Copy Markdown

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

Signed-off-by: duyanghao <1294057873@qq.com>
Signed-off-by: duyanghao <1294057873@qq.com>
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 26, 2019
… 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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants