-
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
Consolidation of changes related to session duration #296
Consolidation of changes related to session duration #296
Conversation
- For creation, duration is made required. Maximum and default are deprecated, relying on the QoS Profile API for any limit. Implementations can grant the requested duration or set a different value in the response. - maxDuration in QoS Profiles is assumed to be the absolute maximum duration including any extensions. That is, extensions can extend the current session duration to the maximumDuration but no longer. Session info changes: - dates are formatted as string, date-format. - startedAt and expiredAt are both optional and not expected to be returned when qosStatus is "REQUESTED". - duration is the overall session duration, including any extension. It should be the interval between startedAt and expiresAt, so it is redundant, unless the qosStatus is "REQUESTED". In this case it would reflect the requested or granted duration. For sessions with qosStatus = "UNAVAILABLE", it must be adjusted to the effective duration.
Fixing linting complaints about trailing spaces
From a developer point of view this looks good. An internal PoC was successfully created. |
Session duration in seconds. Implementations can grant the requested session duration or set a different duration, based on network policies or conditions. | ||
- When `qosStatus` is "REQUESTED", the value is the duration to be scheduled, granted by the implementation. | ||
- When `qosStatus` is AVAILABLE", the value is the overall duration since `startedAt. When the session is extended, the value is the new overall duration of the session. | ||
- When `qosStatus` is "UNAVAILABLE", the value is the overall effective duration since `startedAt` until the session was terminated. | ||
type: integer | ||
format: int32 | ||
minimum: 1 |
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.
Do we need this minimum
definition? My proposal is to omit it: QoS profile is defining beside maxDuration
also minDuration
. From my perspective "Implementations can grant the requested session duration or set a different duration" applies also to the minimum.
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.
It only enforces that duration > 0 (no zero or negative values allowed). minDuration
is optional in the QoS Profile. But any implementation would reject values <=0, even if this not explicitly added to the spec.
- `qosStatus` as `UNAVAILABLE`, and, | ||
- `statusInfo` as `DURATION_EXPIRED`. | ||
See notification callback. | ||
Requested session duration in seconds. Value may be explicitly limited for the QoS profile, as specified in the [Qos Profile API](TBC). Implementations can grant the requested session duration or set a different duration, based on network policies or conditions. | ||
type: integer | ||
format: int32 | ||
minimum: 1 |
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.
Same comment as above. If the minimum is just meant to avoid negative values, I'm fine.
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.
Exactly
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.
@jlurien thanks for the good PR. The comments above are not hindering me to approve it. They are just meant for consideration.
@eric-murray @RandyLevensalor Last meeting we discussed to merge if there are no objections. Do you have checked as well? |
What type of PR is this?
What this PR does / why we need it:
Clarification and consolidation of changes affecting management of session duration.
Session info changes:
Which issue(s) this PR fixes:
Fixes #291 #249 #266 #280 #281 #288
Special notes for reviewers:
Discussion in #291
Changelog input