You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by thvasilo <gi...@git.apache.org> on 2015/03/05 18:18:19 UTC
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
GitHub user thvasilo opened a pull request:
https://github.com/apache/spark/pull/4916
[EC2] [SPARK-6188] Instance types can be mislabeled when re-starting cluster with default arguments
As described in https://issues.apache.org/jira/browse/SPARK-6188 and discovered in https://issues.apache.org/jira/browse/SPARK-5838.
When re-starting a cluster, if the user does not provide the instance types, which is the recommended behavior in the docs currently, the instance will be assigned the default type m1.large. This then affects the setup of the machines.
This solves this by getting the instance types from the existing instances, and overwriting the default options.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/thvasilo/spark SPARK-6188]-Instance-types-can-be-mislabeled-when-re-starting-cluster-with-default-arguments
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4916.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4916
----
commit 3ebd52a8ce5c655e133387d99ea38147cf10efff
Author: Theodore Vasiloudis <th...@users.noreply.github.com>
Date: 2015-03-05T17:08:59Z
Make sure that the instance type is correct when relaunching a cluster.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77818222
[Test build #28387 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28387/consoleFull) for PR 4916 at commit [`6705b98`](https://github.com/apache/spark/commit/6705b9817e6206efddf065bf1b1d7079a8a25276).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77555918
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28339/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77557612
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28341/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77781120
@srowen I'm fine with the line either way and it shouldn't really matter. As I described above the problem of master_instance_type being different from slaves is a different issue not related to this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77555917
[Test build #28339 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28339/consoleFull) for PR 4916 at commit [`3ebd52a`](https://github.com/apache/spark/commit/3ebd52a8ce5c655e133387d99ea38147cf10efff).
* This patch **fails Python style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/4916
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77750513
I'll wait another day in case the consensus is that this line should indeed change, but it sounds like it is likely fine as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77557500
[Test build #28341 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28341/consoleFull) for PR 4916 at commit [`7b32429`](https://github.com/apache/spark/commit/7b32429073b20c6b95a53b3857dddf8cb3f9690c).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77557606
[Test build #28341 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28341/consoleFull) for PR 4916 at commit [`7b32429`](https://github.com/apache/spark/commit/7b32429073b20c6b95a53b3857dddf8cb3f9690c).
* This patch **fails Python style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77555403
ok to test
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77558169
[Test build #28342 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28342/consoleFull) for PR 4916 at commit [`a3d29fe`](https://github.com/apache/spark/commit/a3d29fe9309f04cea9b4615d8a9c1afa6719af94).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25997522
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
--- End diff --
Yup, this is handled in [get_existing_cluster](https://github.com/thvasilo/spark/blob/SPARK-6188%5D-Instance-types-can-be-mislabeled-when-re-starting-cluster-with-default-arguments/ec2/spark_ec2.py#L617)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77710036
@srowen This change doesn't remove any existing functionality as far as I can see. As @nchammas said if you stopped instances of a certain type you can only get the same type back. Right now we implicitly assumed users would pass the same flag, and this handles the use case where the user doesn't pass the right flag.
I had a minor comment inline, but otherwise this change looks good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77557251
Removed trailing whitespace, was that the style problem?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25939525
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
+ existing_master_type = ""
+ opts.master_instance_type = existing_master_type
--- End diff --
Paging @nchammas and or @shivaram for review as I am really not the best person to ask, but, I had expected that you *could* change the instance type on restart. It may not work for all possible combinations, but it seemed like something this script doesn't have to police -- but also should not prohibit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77557369
You can click through to the result of the Jenkins build to see the error:
```
PEP 8 checks failed.
./ec2/spark_ec2.py:1262:1: W293 blank line contains whitespace
./ec2/spark_ec2.py:1270:1: W293 blank line contains whitespace
```
You can run them locally to double-check your work with `./dev/lint-python`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r26000373
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
--- End diff --
@shivaram I think you are correct, the opts.master_instance_type is not accessed at any point in setup_cluster, so this is redundant.
But I think this could lead to another bug. Since the master gets set up using the slaves' instance type (line [860](https://github.com/thvasilo/spark/blob/SPARK-6188%5D-Instance-types-can-be-mislabeled-when-re-starting-cluster-with-default-arguments/ec2/spark_ec2.py#L860)), it might be mis-configured in the same way. But perhaps this is not a problem, as the master should not be an executor as well.
In that sense deploy_files is a bit problematic currently, as it will deploy files on the master that will be rsynced to the slaves, so in one sense it does the right thing by deploying the files configured as they should be on the slaves, but at the same time, the configuration of the master itself is done with the instance type of the slaves, which could be different from the masters'.
Example:
1. User launches cluster, slaves are m3.xlarge, master is m3.large.
2. User stops and restarts cluster.
3. Files deployed on master are configured for m3.xlarge, these get rsynced to the slaves through setup.sh.
4. Now the slaves have correct configuration files, but the master has been configured as an m3.xlarge instance.
Do you think this can be a problem? Other wise I can just remove the if clause as the setting is unnecessary with the current code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77555808
[Test build #28339 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28339/consoleFull) for PR 4916 at commit [`3ebd52a`](https://github.com/apache/spark/commit/3ebd52a8ce5c655e133387d99ea38147cf10efff).
* This patch merges cleanly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77570242
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28342/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by nchammas <gi...@git.apache.org>.
Github user nchammas commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25999086
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
--- End diff --
Per my comments earlier, this change does not remove existing
functionality. It is currently not possible to change instance types when
starting a stopped cluster.
2015년 3월 7일 (토) 오후 12:08, Theodore Vasiloudis <no...@github.com>님이
작성:
> In ec2/spark_ec2.py
> <https://github.com/apache/spark/pull/4916#discussion_r25997522>:
>
> > @@ -1259,6 +1259,15 @@ def real_main():
> > cluster_instances=(master_nodes + slave_nodes),
> > cluster_state='ssh-ready'
> > )
> > +
> > + # Determine types of running instances
> > + existing_master_type = master_nodes[0].instance_type
>
> Yup, this is handled in get_existing_cluster
> <https://github.com/thvasilo/spark/blob/SPARK-6188%5D-Instance-types-can-be-mislabeled-when-re-starting-cluster-with-default-arguments/ec2/spark_ec2.py#L617>
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/4916/files#r25997522>.
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77581802
@nchammas The instances do come up with the correct type, the problem is more the one described [here]
(https://issues.apache.org/jira/browse/SPARK-5838?focusedCommentId=14349004&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14349004)
In short, while the instances themselves are OK, their setup is done assuming the default instance type, m1.large.
This means that the machines are assumed to have 2 disks, and that leads to problems that are described in in issue [5838](https://issues.apache.org/jira/browse/SPARK-5838), where machines that have one disk end up having shuffle spills in the in the small (8GB) snapshot partitions that quickly fills up and results in failing jobs due to "No space left on device" errors.
I will update issue 6188 to include this information.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25882813
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
+ existing_master_type = ""
+ opts.master_instance_type = existing_master_type
--- End diff --
You are correct. Perhaps giving the user the option to change the instance type is a good middle ground. But mis-representing the instance type by having the default value can cause problems as we saw, without the user's knowledge.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25997245
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
--- End diff --
Dumb question here too, does this assume there exists a running master node? it's referencing the first running master node, but if there aren't any? or is that handled?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by nchammas <gi...@git.apache.org>.
Github user nchammas commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77564954
@thvasilo can you clarify what flow you are seeing this behavior for? I can't reproduce the issue you're reporting.
Here's what I tried:
1. Launch a cluster, setting the instance type to `m3.large`, which is different from the default instance type.
```
./spark-ec2 launch "spark-test" \
--instance-type m3.large \
--slaves 1 \
...
```
2. Stop the cluster.
```
./spark-ec2 stop "spark-test" \
...
```
3. Start the cluster without specifying the instance type.
```
./spark-ec2 start "spark-test" \
...
```
My cluster came back up with the instance types I originally specified, which is the expected behavior.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by nchammas <gi...@git.apache.org>.
Github user nchammas commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77567240
I also tried to change the instance type when starting a stopped cluster. That didn't work, and the instances were brought up with their original instance types.
I think that is potentially something we could improve.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25882335
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
+ existing_master_type = ""
+ opts.master_instance_type = existing_master_type
--- End diff --
Does this not force the instance types to match the existing ones? that's not inherently required, right? I might misunderstand.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r26021043
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
--- End diff --
OK, added a comment indicating why we do this, even though the master instance type it's not used in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77407514
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77828099
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28387/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77570226
[Test build #28342 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28342/consoleFull) for PR 4916 at commit [`a3d29fe`](https://github.com/apache/spark/commit/a3d29fe9309f04cea9b4615d8a9c1afa6719af94).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77688219
I understand the goal to be:
- If I do specify an instance type T on restart, launch instance type T and configure for T regardless of previous instance type
- If I don't specify an instance type, the launch and configure correctly for the previous instance type
This change seems always force the new instance type to match the old instance type though. Is the argument that this should not be allowed any longer? or am I mistaken and it was never allowed?
In any event, of course, whatever the instance type is, it should be configured correctly for that instance type!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25937157
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
+ existing_master_type = ""
+ opts.master_instance_type = existing_master_type
--- End diff --
OK so the use case would be:
1. User starts cluster with some instance type, and later stops it
2. The user now wants to restart the cluster.
3. We can either:
1. Just restart the same instances.
2. Or allow the user to change the instance types by redefining them in the options passed with the start command.
Is 3.ii a viable use-case? Is it possible to change the type of the instance being re-launched, for example in the case where the cluster also had persistent storage .i.e. extra EBS volumes that are still attached to the stopped instances?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25883042
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
+ existing_master_type = ""
+ opts.master_instance_type = existing_master_type
--- End diff --
It makes some sense that the default is to 'same instance type', yes, but not forcing it to be the same one. It could emit an info message if they differ.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r26010428
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
--- End diff --
Good point -- but the problem of `master_instance_type` differing from the slaves and handling the configs cleanly is outside the scope of this PR.
Anyways I'm fine with setting it to `""` or not -- one thing you could do is add a comment saying why we do it as it seems unintuitive.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by thvasilo <gi...@git.apache.org>.
Github user thvasilo commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77696081
> Is the argument that this should not be allowed any longer? or am I mistaken and it was never allowed?
I am also not sure about this, it would be cool to allow the user to change the instance type if that is possible, otherwise just relaunching the same cluster with the same instance types seems like a good compromise.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/4916#discussion_r25999599
--- Diff: ec2/spark_ec2.py ---
@@ -1259,6 +1259,15 @@ def real_main():
cluster_instances=(master_nodes + slave_nodes),
cluster_state='ssh-ready'
)
+
+ # Determine types of running instances
+ existing_master_type = master_nodes[0].instance_type
+ existing_slave_type = slave_nodes[0].instance_type
+ if existing_master_type == existing_slave_type:
--- End diff --
The setting of `master_instance_type = ""` seems unnecessary even if its the same as slave_instance_type ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by nchammas <gi...@git.apache.org>.
Github user nchammas commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77609870
Ah, I see. Thank you for the clarification. I would add this into the PR body, since that becomes part of the commit message if this PR gets merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77828087
[Test build #28387 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28387/consoleFull) for PR 4916 at commit [`6705b98`](https://github.com/apache/spark/commit/6705b9817e6206efddf065bf1b1d7079a8a25276).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [EC2] [SPARK-6188] Instance types can be misla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4916#issuecomment-77696556
I guess I'm trying to figure out if this change subtracts existing functionality. That's potentially controversial and what I would hesitate over.
If it merely doesn't _add_ this functionality, that seems fine to me. I'm not necessarily arguing that changing instance type should be possible, either.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org