You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by logc <gi...@git.apache.org> on 2014/03/13 10:51:21 UTC
[GitHub] spark pull request: Add unit test to spark_ec2 script
GitHub user logc opened a pull request:
https://github.com/apache/spark/pull/134
Add unit test to spark_ec2 script
Add a unittest for a refactored function to destroy cluster. Relies on mock and moto dependencies to avoid sending out EC2 requests.
We noticed at work that the supplied script does not work always when destroying clusters, specially for regions outside "us-east-1". This pull request adds a test on the command. If this is accepted, we can continue debugging why exactly are such clusters not terminated correctly.
Run the test with:
```console
$ python ec2/tests.py
Searching for existing cluster cluster_name...
Terminating master...
Terminating slaves...
Deleting security groups (this will take some time)...
Attempt 1
.
----------------------------------------------------------------------
Ran 1 test in 1.040s
```
Please notice that the test relies on [moto](https://github.com/spulec/moto) and [mock](https://pypi.python.org/pypi/mock) to run. Since I did not know how you handle such dependencies, I have not added a `requirements.txt` or a `setup.py` script, but can update the pull request in that sense.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/logc/spark tlpmaster
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/134.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 #134
----
commit 2e7ca995dce6a8822fed4389be4d69223bd671d2
Author: Luis Osa <lo...@tid.es>
Date: 2014-03-12T15:32:30Z
Add unittest for destroy cluster command
Add a unittest for refactored function to destroy cluster. Relies on
mock and moto dependencies to avoid sending out EC2 requests.
commit 534edcc96e33889275ad278dc3cfaf24faf44604
Author: Luis Osa <lo...@tid.es>
Date: 2014-03-12T15:34:13Z
Add pyc files to gitignore
----
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11935416
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
+
+from boto import ec2
+import mock
+import moto
+
+import spark_ec2
+
--- End diff --
extra newline
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40670367
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14196/
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-45462687
@pwendell what do we do with respect to binary files included with this?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40670316
Merged build started.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41311640
Jenkins, test this please
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11935449
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
+
+from boto import ec2
+import mock
+import moto
+
+import spark_ec2
+
+
--- End diff --
Can you also add some class level comments and instructions on how to run this test here ?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41311755
Merged build started.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11943166
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
+
+from boto import ec2
+import mock
+import moto
+
+import spark_ec2
+
+
--- End diff --
Done
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11961673
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
+
+from boto import ec2
+import mock
+import moto
+
+import spark_ec2
+
--- End diff --
Actually I check code in pyspark as well and we do follow the flake8 defaults of extra newline before classes. So keep this in is absolutely fine.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-44281011
@rxin - I did not intend to bring more attention to this PR ... if you scroll back in the discussion, you can read that I reminded the people reviewing this to keep it on their radar (or dismiss it) at least twice. I know you have lots of requests piling up; I would be also happy with a reject decision. (Well, not exactly happy, but content).
On the issue of binary artifacts, I included them on request of the pull request reviewer, because this part of the project aims to be self-contained. Do I remove them or keep them?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-55552464
Let's close this issue for now and hopefully someone can take it up and bring it up to date.
---
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: Add unit test to spark_ec2 script
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-54694760
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: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11943099
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
+
+from boto import ec2
+import mock
+import moto
+
+import spark_ec2
+
--- End diff --
The extra newline before classes is required by the `flake8` tool in its default settings, which is what I am using to review Python style for this script. If you tell me this is not your enforced style, then I will change it. However, if you do not have any enforced style, please allow me to keep it and suggest flake8 defaults as the style standard.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40670366
Merged build finished.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-45463264
Hey @rxin so I think the "no binaries" means "no compiled code" - I don't think it means we can't have a zip file. These zip files only contain uncompiled `.py` files, so I think it's okay.
That said @logc I noticed the zip files are pretty large (e.g. almost one MB total). They include a lot of docs/html/etc that aren't needed. Would you mind re-packaging them into smaller zips (i.e. just remove the stuff in there that is not needed). I do like having it be self contained.
Also, would you mind actually running these tests in the default test runner? Just add something to to the `dev/run-tests` script at the end. I also couldn't seem to actually get these to run, but maybe I'm not setting up the PYTHONPATH correctly:
```
PYTHON_PATH=ec2/third_party/mock-1.0.1.zip/mock-1.0.1 ./ec2/tests.py
```
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc closed the pull request at:
https://github.com/apache/spark/pull/134
---
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: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40913796
Sorry for the delay in looking at this. The refactoring into a new function `destroy_cluster` looks good to me. I am not sure I understand the purpose of the unit test. If we want to debug AWS consistency issues then those will not occur in the unit test ? Am I missing something ?
Regd. dependencies, we actually ship the boto version we use as a part of Spark in https://github.com/apache/spark/tree/master/ec2/third_party -- What are the licenses of moto and mock ? If they are Apache compatible and not very large, we can add them to third_party.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41148625
@shivaram : sorry, I did not have time to work on this yesterday.
I added the library zip files and license terms as requested. Zipfiles are *for the release I worked with*, not the latest release. Otherwise, as you may notice, `mock` is roughly the same size as the `boto` library and `moto` is substantially less.
The license terms are added right underneath the `boto` license terms (I did not know if there is another ordering you want to keep). I am not a lawyer and I can't assure this is the way to comply with license terms, but it looks reasonable to me.
Out of curiosity: why don't you manage Python dependencies through a `requirements.txt` or a `setup.py`? It would take care of downloading such packages for you, and license terms are met automatically as far as I know ...
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41311732
Merged build triggered.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40924504
Hi @shivaram , thanks for your comments!
The purpose of the unit test is to start defining invariants in the code, and later be sure that changes do not break invariants. That means, even if the ultimate purpose of the code is to start a machine on EC2, we can consider the purpose fulfilled if we send the corresponding request out (leaving the actual start of the machine as a responsibility of AWS). The moto library is in charge of faithfully replicating the AWS interface -- which, to my knowledge, it does -- but avoiding to start actual machines and incur in costs.
The situation is similar to when you mock out your database: you want to check your logic, and leave the intended secondary effects as a concern of the database.
You are right that this first test is quite uninteresting: it just checks that no exceptions are raised. But this was intended as a way to start setting up the test infrastructure. I intend to expand this in order to test other functions; at some point, this test will receive a (mocked) started cluster on EC2, and check that terminating it does eliminate all of its instances.
I am submitting this change set as it is because it is self-contained, and it is complete: others can start developing tests of their own, and the first test does guarantee that other changes do not break the `destroy_cluster` functionality.
Regarding licenses: moto is under the Apache license as you can see [here](https://github.com/spulec/moto/blob/master/LICENSE), and mock is under the BSD license. As far as I know, [BSD and Apache are compatible](http://programmers.stackexchange.com/questions/40561/is-bsd-license-compatible-with-apache). The only restriction I can think of is that you should leave the BSD notice on the library.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41316041
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14442/
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-37518956
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40669823
@shivaram can you take a look at this?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41266700
I just corrected the style comments, except for one where I would like clarification on your style standard.
I understand that the Spark project uses mainly SBT for construction, installation and testing. Probably you do not want to mix in another build system for the Python part (in the sense of managing dependencies). However, please make the project aware that the second mechanism I propose --defining a `setup.py`-- works without the user needing anything except a Python interpreter (which is anyway needed just to launch the script).
And let me also mention that I reviewed the Travis error on my previous commit, and it seems unrelated to my changes: an SBT test for `BagelSuite`, whatever that is, does *not* halt after a `large number of iterations`.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41367014
Yes, the dependencies have to be included in PYTHONPATH. I installed them on my machine using `pip`, and included the zip files as requested. If you would like to work solely from the zip files, then you should extract them and run `python setup.py install` in each of the extracted source trees.
Again, this is something that would be automated if this script had its own `setup.py`, but I don't want to sound repetitive :smile:
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-38881894
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-44281268
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-51955535
This PR is a pretty neat idea; it would be great to get more testing of the Spark EC2 scripts, especially as we add new features (like #1899). Does anyone want to adopt this code and bring it up-to-date with the latest version of Spark? If not, I might be able to do it myself as soon as I'm a little less busy.
---
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: Add unit test to spark_ec2 script
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40669772
Jenkins, test this please.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc closed the pull request at:
https://github.com/apache/spark/pull/134
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41602709
@logc -- Sorry I am caught with with a deadline and didn't get a chance to look at this. I think the basic idea is that we don't want users to run setup.py to use spark_ec2.py. This script is meant to be a utility script that just works out of the box without any installation. This is part of the reason why we ship the dependencies and include them in the PYTHONPATH in the shell script `spark-ec2`.
Anyways I think this change looks good. We could add a `run-tests`method that will put moto and mock in the PYTHONPATH, but I am not sure that is very necessary as these tests won't be executed by all users.
@pwendell can you take a look before merging ?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41241727
Thanks @logc -- I had some style comments that I left inline. The dependencies + License look good to me.
I am not really sure why we dont use `requirements.txt` / `setup.py` -- One reason I can see is that we want it to be zero overhead for users to run spark_ec2.py without installing or setting up anything before that.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-43950391
(And yes - please re-open it) Thanks!
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-37825730
If I may clarify this pull request in any way, please ask me :smile:
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40962343
@logc I see, so the goal is to check for invariants in spark_ec2.py to detect bugs etc. Okay that sounds good.
Can you create a zip file for moto, mock similar to the boto zip file we include in third_party ? If the size of the zip file is small, I think this should be fine.
And the license stuff looks good. It'll be great if you can add the BSD parts to https://github.com/apache/spark/blob/master/LICENSE (similar to boto)
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41316039
Merged build finished. All automated tests 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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-43883653
Since this pull request does not seem to get much attention, I am closing it. Thank you for your time!
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11943162
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
--- End diff --
Done
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-38899714
It seems there is no interest in my proposal. If the Jenkins server complains again in 2 weeks, I will just close the PR. Thanks.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
GitHub user logc reopened a pull request:
https://github.com/apache/spark/pull/134
Add unit test to spark_ec2 script
Add a unittest for a refactored function to destroy cluster. Relies on mock and moto dependencies to avoid sending out EC2 requests.
We noticed at work that the supplied script does not work always when destroying clusters, specially for regions outside "us-east-1". This pull request adds a test on the command. If this is accepted, we can continue debugging why exactly are such clusters not terminated correctly.
Run the test with:
```console
$ python ec2/tests.py
Searching for existing cluster cluster_name...
Terminating master...
Terminating slaves...
Deleting security groups (this will take some time)...
Attempt 1
.
----------------------------------------------------------------------
Ran 1 test in 1.040s
```
Please notice that the test relies on [moto](https://github.com/spulec/moto) and [mock](https://pypi.python.org/pypi/mock) to run. Since I did not know how you handle such dependencies, I have not added a `requirements.txt` or a `setup.py` script, but can update the pull request in that sense.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/logc/spark tlpmaster
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/134.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 #134
----
commit 2e7ca995dce6a8822fed4389be4d69223bd671d2
Author: Luis Osa <lo...@tid.es>
Date: 2014-03-12T15:32:30Z
Add unittest for destroy cluster command
Add a unittest for refactored function to destroy cluster. Relies on
mock and moto dependencies to avoid sending out EC2 requests.
commit 534edcc96e33889275ad278dc3cfaf24faf44604
Author: Luis Osa <lo...@tid.es>
Date: 2014-03-12T15:34:13Z
Add pyc files to gitignore
commit 4b2d264d6bca69b4cac3e5e99b509871051af739
Author: Luis Osa <lo...@tid.es>
Date: 2014-04-23T10:54:01Z
Add license terms and zipfiles for dependencies
commit a58bc0c3992f21bce954e7c9f2b4f3738b5744dc
Author: Luis Osa <lo...@tid.es>
Date: 2014-04-24T10:44:27Z
Add shebang line, header and docstrings
----
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on a diff in the pull request:
https://github.com/apache/spark/pull/134#discussion_r11935429
--- Diff: ec2/tests.py ---
@@ -0,0 +1,25 @@
+import unittest
--- End diff --
This should probably include
```
#!/usr/bin/env python
# -*- coding: utf-8 -*-
```
and definitely include the Apache license header
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-40669877
Merged build triggered.
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by shivaram <gi...@git.apache.org>.
Github user shivaram commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41311622
Thanks @logc for making the changes. The travis error can be ignored (we are still testing out the Travis setup). We do use Jenkins to test things before merging, so I am going to trigger that now.
One question about running tests: Do we need to somehow include the dependencies in the PYTHONPATH ? Or does running python tests.py automatically get them in somehow ?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by logc <gi...@git.apache.org>.
Github user logc commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-41544754
@shivaram : I hope all is well with my last PYTHONPATH answer ... ?
---
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.
---
[GitHub] spark pull request: Add unit test to spark_ec2 script
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-67102486
/bump. Let's see if we can get the auto-close script to pick up this JIRA.
close this issue
close this pr
close this please
---
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: Add unit test to spark_ec2 script
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/134#issuecomment-43950372
Actually no need to close this. A reminder would be fine (as you can see, there are lots of pull requests that are piling up, the rate of them coming in is higher than the rate of review).
I think this is good to have in general, but one thing is we cannot include binary artifacts (even zip files) in Spark code base (this is a rule by Apache Software Foundation). Is there anyway we can work around that?
---
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.
---