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

Specify provider state after shutdown() #213

Closed
toddbaert opened this issue Nov 3, 2023 · 8 comments · Fixed by #216
Closed

Specify provider state after shutdown() #213

toddbaert opened this issue Nov 3, 2023 · 8 comments · Fixed by #216

Comments

@toddbaert
Copy link
Member

toddbaert commented Nov 3, 2023

We don't currently specify the state of the provider after a shutdown().

I'm proposing we should explicitly require (MUST) or suggest (SHOULD) the provider transitions back into either NOT_READY (implying that it can be re-initialized) or READY (for a provider not requiring initialization) once it has been shut down. I think this is a fairly sensible approach, and will remove this ambiguity in a clean way. Some providers already work this way.

Essentially we'd be adding another transition in the state diagram here from READY to NOT_READY via shutdown.

@kinyoklion
Copy link
Member

This effectively disallows providers that would hit some terminal state, they are neither ready, nor could they be initialized again. Potentially this is fine, but I think it is worth an explicit callout.

@toddbaert
Copy link
Member Author

toddbaert commented Nov 3, 2023

This effectively disallows providers that would hit some terminal state, they are neither ready, nor could they be initialized again. Potentially this is fine, but I think it is worth an explicit callout.

I agree, and think that might be optimal. If we go this route, we can note that. Our state diagrams will also be circular, which will communicate the same thing.

@justinabrahms
Copy link
Member

+1

@weyert
Copy link
Contributor

weyert commented Nov 4, 2023

In my opinion shutdown is a final state so no need to recover from by going into a NOT_READY state. If they want to recover from it they can recreate the provider?

@lukas-reining
Copy link
Member

I would be fine with either a terminal state or the requirement for all providers to be able to be reenitialized which would imply the two states READY or NOT_READY as you said @toddbaert.
First I was leaning towards the terminal state but I think as every provider has to be initialized anyways, I would go for beeing able to reenitialize, which should be doable in any case (by just running everything from constructor and a full cleanup in the shutdown)

@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented Nov 6, 2023

I am voting for recommending going into a NOT_READY state.

When shutdown is called, the provider is meant to clean up its resources (as per spec The provider MAY define a mechanism to gracefully shutdown and dispose of resources. ). This clearly make them NOT_READY to serve flag evaluations. And the status changes beyond that are out of scope of the OF spec.

@toddbaert
Copy link
Member Author

In my opinion shutdown is a final state so no need to recover from by going into a NOT_READY state. If they want to recover from it they can recreate the provider?

That was certainly the case before. The problem is the lack of any prescription here makes things ambiguous. It seems to me that we'd like to make the provider lifecycle as consistent as possible - it's already one of the trickiest areas for provider and SDK implementors.

I think we have majority support for shutdown transitioning back to NOT_READY, and a lot of providers behave this way already. I will open a spec PR for this.

@toddbaert
Copy link
Member Author

@lukas-reining @kinyoklion @justinabrahms and others, please take a look at the associated spec PR if you get a sec: #216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants