search

nchammas / flintrock

A command-line tool for launching Apache Spark clusters.
Apache License 2.0
637 stars 116 forks source link

Support spot request duration when adding slaves #321

Closed mblackgeo closed 3 years ago

mblackgeo commented 3 years ago

This PR follows up on #315 adding the spot request duration to "add slaves".

I have ran the the tests and the only failure was flake8. I was not able to run the acceptance tests however (I think it's a configuration issue on my end but I don't have a lot of time to dig into it right now). I did manually launch / add slaves / remove slaves / destroy, and verified the valid until date of a spot requested cluster is correct in the EC2 dashboard.

Apologies I did not cover this in the first PR!

nchammas commented 3 years ago

It looks like you opened this PR against a version of master that is somehow disconnected from the repo's actual master. I'm not sure how that happened, but the result is a weird merge conflict.

To clear this without engaging in any serious git surgery, it would probably be simplest to just delete and re-establish your fork of the repo, and then open a new PR from a branch, not master. (Notice how the PR is from mblack20:master to nchammas:master. Normally, you want the pattern to be something like: PR from mblack20:some-feature-branch to nchammas:master.)

Anyway, to save you the trouble I just went ahead and reimplemented the fix in #328, along with some additional tweaks.

Thanks for following up on the original PR!

mblackgeo commented 3 years ago

Thanks, and apologies for the git troubles. Not sure what exactly happened with my fork of the repository in the end!