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

Geofencing - Adding SUBSCRIPTION_DELETED as a Termination Reason #141

Closed
bigludo7 opened this issue Jan 17, 2024 · 9 comments · Fixed by #242
Closed

Geofencing - Adding SUBSCRIPTION_DELETED as a Termination Reason #141

bigludo7 opened this issue Jan 17, 2024 · 9 comments · Fixed by #242
Assignees
Labels
enhancement New feature or request

Comments

@bigludo7
Copy link
Collaborator

Problem description
Feedback from implementation.
For the terminationReason we have following value:

  • NETWORK_TERMINATED - API server stopped sending notification
  • SUBSCRIPTION_EXPIRED - Subscription expire time (required by the requester) has been reached.

We did not have a specific value when the termination was triggered from a DELETE operation.

We can assume that because the consume side perform the DELETE no need for notification but for sake of consistency send an event seems to be a good option.

Possible evolution
Add value SUBSCRIPTION_DELETED in the terminationReason array

Alternative solution

Additional context

@bigludo7 bigludo7 added the enhancement New feature or request label Jan 17, 2024
@jlurien
Copy link
Collaborator

jlurien commented Jan 17, 2024

This may have sense, but it is something that would apply to subscriptions for all APIs, wdyt @PedroDiez ?

@alpaycetin74
Copy link
Collaborator

alpaycetin74 commented Jan 26, 2024

If we assume deletion of subscription is a synchronous operation, successful response to DELETE operation is sufficient.
If we believe the deletion of subscription in the backend must be done asynchronously, a notification is necessary because successful response to DELETE would only mean the backend received & accepted the request.

To be flexible, we can support both. But then we should define 2 different success responses to DEL so that the client would now if deletion is completed or they should wait for a notification.

Having said that, we could also make creation of subscription asynchronous (HTTP 201 created followed by a notification later).
But we never considered it. Maybe it is also not necessary for DEL ?

@bigludo7
Copy link
Collaborator Author

Hello @alpaycetin74
For me the question is not about the fact that the DELETE is performed async or sync. As we send a 204 the deletion is sync for the system triggering the DELETE.

But we cannot assume technical correlation between the system listening to the event channel (the target of the notificationUrl) and the system performing the DELETE (OK this is the problem of the consumer).
But as we are committed to send a suscription-ends notification to the listener, the point for me is to provide information about subscription termination reason to this listener.

Does it make sense?

@alpaycetin74
Copy link
Collaborator

hello @bigludo7 ,
yes, especially when thinking of _"we cannot assume technical correlation between the system listening to the event channel (the target of the notificationUrl) and the system performing the DELETE" it makes sense. It could help the listener application to shut down, maybe save some resources/cost/battery.

@PedroDiez
Copy link

PedroDiez commented Feb 7, 2024

This may have sense, but it is something that would apply to subscriptions for all APIs, wdyt @PedroDiez ?

Yes, think makes sense for APIs that have explicit subscriptions model (i.e. Resource-based). Think this is something to be indidated/align in commonalities

@bigludo7
Copy link
Collaborator Author

bigludo7 commented Mar 6, 2024

Hello
Perhaps we can move forward on this as same event type hase been added in QoD API & Device Status API (camaraproject/DeviceStatus#117).

To be aligned with this API we should use DELETE_REQUESTED wording.

@alpaycetin74
Copy link
Collaborator

I have seen @akoshunyadi 's comment saying "DELETE_REQUESTED" sounds like it is not final, and still some final event is to come. I'd prefer SUBSCRIPTION_DELETED as well.

@bigludo7
Copy link
Collaborator Author

bigludo7 commented Mar 6, 2024

Hello @alpaycetin74 ....yes agreed with you of the interpretation but probably the idea is to have consistence with QoD API. (https://github.com/camaraproject/QualityOnDemand/blob/v0.10.0/code/API_definitions/qod-api.yaml#L918).

Conclusion: make sense to set this at commonalities level to have a de facto alignement.

@akoshunyadi
Copy link
Contributor

The discussion about subscription lifecycle management in commonalities: camaraproject/Commonalities#153

@maxl2287 maxl2287 changed the title Geofencing - Adding a value in Termination Reason value enum Geofencing - Adding SUBSCRIPTION_DELETED as a Termination Reason Jul 29, 2024
@maxl2287 maxl2287 self-assigned this Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants