[SPARK-5668] Display region in spark_ec2.py get_existing_cluster() #4457

MiguelPeralvo · 2015-02-08T10:39:05Z

Show the region for the different messages displayed by get_existing_cluster(): The search, found and error messages.

AmplabJenkins · 2015-02-08T10:42:09Z

Can one of the admins verify this patch?

srowen · 2015-02-08T11:13:51Z

ok to test

SparkQA · 2015-02-08T11:19:15Z

Test build #27039 has started for PR 4457 at commit 4ecd9f9.

This patch merges cleanly.

SparkQA · 2015-02-08T12:39:37Z

Test build #27039 has finished for PR 4457 at commit 4ecd9f9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-08T12:39:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27039/
Test PASSed.

nchammas · 2015-02-08T22:34:47Z

ec2/spark_ec2.py

Really minor point, but I would leave this print statement unchanged since users are unlikely to take action at this particular point, and we printed the region earlier.

@nchmmas, I was wondering the same. I'll remove it when I review it. Thank you.

nchammas · 2015-02-08T22:37:00Z

@MiguelPeralvo Do you think we should print the region when launching a cluster, or does that already happen?

Also, I think we should update the help text for the --region option to indicate that this is the region used to find instances, not just launch them.

MiguelPeralvo · 2015-02-09T16:13:44Z

@nchammas,

Regarding the help text, I'd say that it applies to the destroy, login, reboot-slaves, get-master, stop and start options, not only launch. Something like "EC2 region to launch/destroy/login/reboot-slaves/get-master/stop/start instances in" or "EC2 region where to apply action to: launch/destroy/login/reboot-slaves/get-master/stop/start instances in" is a potential text, but I'm happy to apply any other that reflects better where the parameter region applies, maybe "EC2 region used to find instances in or to launch them in", as you proposed (all the other actions involve the search).

Regarding the region being printed when launching a cluster: As the "get_existing_cluster()" method gets used by all actions, the region will get printed for all of them if the proposed change is applied. I'm in favor of the region being printed out. For example, this would be the beginning of the output of a launch execution with the region change being applied:

Miguels-MacBook-Pro:ec2 Miguel$ $SPARK_HOME/ec2/spark-ec2 -k xyzapachespark -i $SSH_HOME/xyzapachespark.pem -r eu-west-1 launch my-spark-cluster

Setting up security groups...
Searching for existing cluster my-spark-cluster in region eu-west-1...
Spark AMI: ami-1ae0166d
Launching instances...
Launched 1 slaves in eu-west-1c, regid = r-679b8425
Launched master in eu-west-1c, regid = r-e69d82a4
...

If we don't apply the proposed change to the "get_existing_cluster()" method, the zone appears, but not the region. For example, this would be the beginning of the output of a launch execution:

Setting up security groups...
Searching for existing cluster my-spark-cluster ...
Spark AMI: ami-1ae0166d
Launching instances...
Launched 1 slaves in eu-west-1c, regid = r-679b8425
Launched master in eu-west-1c, regid = r-e69d82a4
...
Warning: Permanently added 'ec2-54-194-24-152.eu-west-1.compute.amazonaws.com,54.194.24.152' (RSA) to the list of known hosts.

I've tested launch with the default region, without the region change and when the key pair is not found in the default region, this is the output I get from spark-ec2 (again, it makes it a little bit difficult for a new user to figure out what's wrong):

Setting up security groups...
Searching for existing cluster my-spark-cluster...
Spark AMI: ami-5bb18832
Launching instances...
ERROR:boto:400 Bad Request
ERROR:boto:<?xml version="1.0" encoding="UTF-8"?>
<Response><Errors><Error><Code>InvalidKeyPair.NotFound</Code><Message>The key pair 'xyzapachespark' does not exist</Message></Error></Errors><RequestID>7ef88166-5cee-4c2a-acdc-f77ebe34acfc</RequestID></Response>
Traceback (most recent call last):
...

Please, feel free to change the wording for --region, if required.

SparkQA · 2015-02-10T09:17:27Z

Test build #27191 has started for PR 4457 at commit 3923f36.

This patch merges cleanly.

SparkQA · 2015-02-10T09:18:26Z

Test build #27191 has finished for PR 4457 at commit 3923f36.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-10T09:18:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27191/
Test FAILed.

Removed trailing space from line 611 to make it PEP 8 compliant.

SparkQA · 2015-02-10T09:27:30Z

Test build #27192 has started for PR 4457 at commit 0a837b0.

This patch merges cleanly.

SparkQA · 2015-02-10T10:38:01Z

Test build #27192 has finished for PR 4457 at commit 0a837b0.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-10T10:38:04Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27192/
Test FAILed.

MiguelPeralvo · 2015-02-10T16:17:43Z

The MiMa fail may be flaky. It passed in my computer.
Jenkins, retest this please.

srowen · 2015-02-10T16:34:41Z

ec2/spark_ec2.py

Nit: "to to". Yes the Mima result must be a false positive given the nature of this change.

@srowen

Fixed typo found by @srowen "to to".

SparkQA · 2015-02-10T16:52:27Z

Test build #27211 has started for PR 4457 at commit a5514c8.

This patch merges cleanly.

SparkQA · 2015-02-10T18:05:35Z

Test build #27211 has finished for PR 4457 at commit a5514c8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-10T18:05:39Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27211/
Test PASSed.

srowen · 2015-02-10T18:31:51Z

Looks good. If there aren't any additional comments, I can merge this soon

nchammas · 2015-02-10T19:37:32Z

LGTM

[SPARK-5668] Display region in spark_ec2.py get_existing_cluster()

4ecd9f9

Show the region for the different messages displayed by get_existing_cluster(): The search, found and error messages.

nchammas reviewed Feb 8, 2015
View reviewed changes

Update spark_ec2.py

3923f36

Please, feel free to change the wording for --region, if required.

Update spark_ec2.py

0a837b0

Removed trailing space from line 611 to make it PEP 8 compliant.

srowen reviewed Feb 10, 2015
View reviewed changes

ec2/spark_ec2.py Outdated

Copy link

Member

srowen Feb 10, 2015

Choose a reason for hiding this comment

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

Nit: "to to". Yes the Mima result must be a false positive given the nature of this change.

Update spark_ec2.py

a5514c8

Fixed typo found by @srowen "to to".

asfgit closed this in c49a404 Feb 10, 2015

MiguelPeralvo deleted the patch-2 branch February 10, 2015 20:00

[SPARK-5668] Display region in spark_ec2.py get_existing_cluster() #4457

[SPARK-5668] Display region in spark_ec2.py get_existing_cluster() #4457

Uh oh!

Conversation

MiguelPeralvo commented Feb 8, 2015

Uh oh!

AmplabJenkins commented Feb 8, 2015

Uh oh!

srowen commented Feb 8, 2015

Uh oh!

SparkQA commented Feb 8, 2015

Uh oh!

SparkQA commented Feb 8, 2015

Uh oh!

AmplabJenkins commented Feb 8, 2015

Uh oh!

nchammas Feb 8, 2015

Choose a reason for hiding this comment

Uh oh!

MiguelPeralvo Feb 9, 2015

Choose a reason for hiding this comment

Uh oh!

nchammas commented Feb 8, 2015

Uh oh!

MiguelPeralvo commented Feb 9, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

AmplabJenkins commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

AmplabJenkins commented Feb 10, 2015

Uh oh!

MiguelPeralvo commented Feb 10, 2015

Uh oh!

srowen Feb 10, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

SparkQA commented Feb 10, 2015

Uh oh!

AmplabJenkins commented Feb 10, 2015

Uh oh!

srowen commented Feb 10, 2015

Uh oh!

nchammas commented Feb 10, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants