search

nchammas / flintrock

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

Wait for terminated state while destroying a cluster #231

Closed alexandrnikitin closed 6 years ago

alexandrnikitin commented 6 years ago

This PR makes the following changes:

I tested this PR by manually launching and destroying clusters. I'm not sure about tests for this, should I add one to cover --no-wait option?

Fixes #229

nchammas commented 6 years ago

Hey @alexandrnikitin, thanks for submitting this PR! After thinking about this a bit, I think it's fine to just change the default behavior to wait for termination. I'd do that right after here.

To explain my thought process, wait_for_state() is a method on the EC2 implementation of FlintrockCluster, so we shouldn't use it in the provider-agnostic code in flintrock.py or core.py. Instead of figuring out how to enhance the internal API to capture this new feature correctly and delineate between EC2-specific and provider-agnostic code, I think it's fine to just change the default behavior.

How long does the additional wait typically take for a 10-node cluster? If it's a long time, perhaps we should tweak the text here to inform users that the operation may take some time to complete.

alexandrnikitin commented 6 years ago

Ok, sounds good to me. I'll create another PR for that.

How long does the additional wait typically take for a 10-node cluster?

It's around a dozen of seconds. I think the message we have right now is enough.

nchammas commented 6 years ago

Okie doke. Just FYI for the future, you don't need to open a new PR (#232). Additional commits on the same PR are OK since they will all be squashed when I merge.