-
Notifications
You must be signed in to change notification settings - Fork 60
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
Added clarification of notifications to be sent and added StatusInfo "DELETE_REQUESTED" to addres #241 #258
Added clarification of notifications to be sent and added StatusInfo "DELETE_REQUESTED" to addres #241 #258
Conversation
Co-authored-by: Shilpa Padgaonkar <[email protected]>
In my understanding, the PR is not directly addressing the issue description:
We are not indicating that the UNAVAILABLE notification does not delete the session automatically. The PR addresses that notifications will be sent even after the session is deleted by the user. But in case of unavailability due to network terminated, this is another case. |
@jlurien You are right. I have only addressed yet part of the derived conclusion from our discussion in the call on Dec 15th (added to the issue):
This design target was the reason to not delete the QoD session resource implicitly after sending the But we also discussed that we can't expect that a client will always send an explicit delete for such a session resource - it's not even clear if a client would recognise the situation, e.g. if it was set up for a specific duration and the client hasn't provided a callback. We can also not rely on the expiration of the defined session duration, as this would be done by the network and the related network session is already terminated / not anymore available The discussed solution was therefore to keep the session resource for a fixed time span after it got unavailable due to "NETWORK_TERMINATED". E.g. 6 minutes to be sufficient for clients which poll each 5 minutes the status. I will add the description of this discussed solution. We haven't discussed the behavior for "DURATION_EXPIRED" - my understanding is that in this case the session resource will be implicitly also deleted as this is the expected behavior and no longer exist after this event. Any other opinion? |
Added behavior in case of `NETWORK_TERMINATED` and the need to delete a session explicitly after such event with the time period of 360 seconds before a new QoS session for the same device and flow can be created. In addition clarified that there will be no notification event if a session with qosStatus `UNAVAILABLE` will be deleted.
Added the behavior in case of Note: the 360 seconds are just an initial proposal (to allow a poll period up to 300 seconds). In addition clarified that there will be no notification event if a session with qosStatus |
@jlurien @shilpa-padgaonkar @maxl2287 @RandyLevensalor @eric-murray would be great if you could review this PR (again) before the QoD call tomorrow. It would be good if we can decide tomorrow about a second release candidate of v0.10.0 which includes this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments below regarding the notes below.
LGTM |
Added clarification that the automatic deletion of session resources after `NETWORK_TERMINATED` will happen at earliest after 360 seconds but could happen also later.
As agreed within our call I've added the clarification that the automatic deletion of session resources after @jlurien @maxl2287 @RandyLevensalor @eric-murray Please do another review/approval early within the week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Adding clarification that notification will be sent for all changes of qosStatus and what happens when qosStatus change from AVAILABLE to UNAVAILABLE
Which issue(s) this PR fixes:
Fixes #241 and #254
Special notes for reviewers:
As notification should be sent in all QosStatus changes, there was the need to add the StatusInfo "DELETE_REQUESTED".
Changelog input