search

Apicurio / srs-fleet-manager

Implementation of the Managed Service Registry Service API (aka control plane API).
Apache License 2.0
4 stars 13 forks source link

Async subscription creation #337

Closed carlesarnal closed 1 year ago

carlesarnal commented 1 year ago

@jsenko there are still a few things I want to polish but should be mostly ok to review now. Basically, I have added a new task in front of the ScheduleRegistryTask that checks the terms, creates the subscription etc.

carlesarnal commented 1 year ago

This looks good, however there is an issue with reporting errors from async tasks. For example the org being out of available subscriptions. I was thinking on adding an error field into the Registry schema component, or more generally an extended status. This must be correctly handled in the UI as well.

With the current approach, provisioning the subscription is going to be retried and, if retries are exhausted, the Registry instance will eventually be deleted. After this, if the user goes to the UI, the UI will display the message about the quota consumed if that condition is still true or, if it's not, the user can retry creating it. What I'm trying to bring to light is that this scenario will only happen if the quota is exhausted (or if there's an error provisioning it, but I'm working on improving the flow) during the period between accessing the UI and the instance creation. @jsenko let me know what you think about this.

@EricWittmann I'm also interested in knowing your opinions about this since this might change/impact the current experience a little bit.

jsenko commented 1 year ago

CLI would have to check for quota as well. We can rely on UI in short-term, but I think it's important to have a way to communicate async errors to users. Otherwise we may start getting requests to investigate an expected situation when they fail to provision an instance.

carlesarnal commented 1 year ago

CLI would have to check for quota as well. We can rely on UI in short-term, but I think it's important to have a way to communicate async errors to users. Otherwise we may start getting requests to investigate an expected situation when they fail to provision an instance.

Ok, what about introducing a FAILED_TO_PROVISION state or something similar and allowing deletion of those instances? We can also add a new field to the RegistryData as you were suggesting to add the reason so both, the UI and the CLI can communicate what's the problem properly. Does that makes sense?

carlesarnal commented 1 year ago

As discussed, I am now setting the instance as failed and adding a failed reason field to the registry instance data

carlesarnal commented 1 year ago

@jsenko with the latest changes do you have any objection to merging this?

carlesarnal commented 1 year ago

@jsenko I have addressed both comments, let me know if you think it's ok or if something else should be addressed.

jsenko commented 1 year ago

@jsenko I have addressed both comments, let me know if you think it's ok or if something else should be addressed.

will do, thanks