Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent new instances from starting when Cluster.stop() had been issued #699

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

shadeofblue
Copy link
Contributor

closes #461 ?

@shadeofblue shadeofblue requested review from a team and johny-b September 30, 2021 16:10
@shadeofblue shadeofblue self-assigned this Sep 30, 2021
Copy link
Contributor

@johny-b johny-b left a comment

Choose a reason for hiding this comment

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

This seems to work, although I can't say I really like the solution.
Alternative approach would be to add just a _stopped attribute, without any statemachine.

I think that when we do this sort of a "heavy" change (additional class, additional logic implemented on this class), it would be better to at least have it somewhat complete. E.g. Cluster definitely has a state "stopped", but ClusterState has no such option.

IOW, this is not really Cluster.state, rather Cluster.state_in_few_situations_we_needed_it_and_something_useless_otherwise.

@johny-b
Copy link
Contributor

johny-b commented Oct 1, 2021

ALSO: the only reason we need some ClusterState is that we don't have any instance handlers immediately, only after the activity starts. When #560 is done this will be redundant, instead of if self.state == ClusterState.running: we'll have if not instance.stopped:.

And I think that if we could stick to the state of instance only, without state of Cluster, this would be neat.

@johny-b johny-b self-requested a review October 4, 2021 12:25
Copy link
Contributor

@johny-b johny-b left a comment

Choose a reason for hiding this comment

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

image

@shadeofblue shadeofblue merged commit efa9b1c into b0.7 Oct 4, 2021
@shadeofblue shadeofblue deleted the blue/dont-spawn-instances-on-cluster-stop branch October 4, 2021 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

immediately stopped cluster will still start instances
3 participants