Closed RandyLevensalor closed 1 year ago
@eric-murray I think that I've incorporated your feedback into the PR. But there's a decent chance that I missed something. Thanks!
@RandyLevensalor Could you make a list of the topics we need to discuss tomorrow in our call - best with the options we have? (and try to resolve the others before the call, just if possible)
@hdamker I'll get the updated PR posted this evening.
@eric-murray thanks for all of your comments and feedback. I'm working on incorporating it.
@hdamker
Open items to discuss:
@RandyLevensalor thanks for the summary of open issues
4. Do these need to be duplicated in the documents as well?
No, see #144 (but we might want to keep the mapping table seperat).
@eric-murray @hdamker @jlurien @ToshiWakayama-KDDI
I've created a new issue to begin the discussion around extending the qos-profile queries. Please chime in there with any thoughts. #147
Discussion results from call at 2023-05-05
- Behaviour and parameters of querying QoS Profiles. The latest version is very limited and can be updated in a subsequent release.
Only simple queries within this PR (and v0.9.0), for further enhancements @RandyLevensalor has already opened #147
- Is mimumumTargetRate acceptable or would guaranteed be more better.
We keep the term minimumTargetRate. Within mobile networks a value >0 can be interpreted as guaranteed bitrate (but still the technical implementation could be use other mechanisms as well).
- The latest draft eliminates the unique ID in favor of a unique name with minimal constraints. Is this the approach that we want to take?
We taking the apporach id = name. It would be good to have naming conventions to distinct profiles aligned across operators from operator specific ones going forward.
- Descriptions in the OpenAPI spec have been revised. Which ones need additional clarifications? Do these need to be duplicated in the documents as well?
No need to duplicate, see #144
@RandyLevensalor would it be possible to resolve the remaining comments before Friday?
Some additional comments:
targetMin*
may reflect the tentative bottom limit (avoiding calling it guaranteed). but what is the difference between max*
and maxBurst*
from a developer perspective.description
to allow some dev friendly qualitative explanation, e.g,. "Low jitter, low packet losses, low latency.", "Real Time gaming" ... @RandyLevensalor Would it be possible to resolve the comments from @eric-murray or do you need more input on them?
@eric-murray @jlurien Thanks for the additional feedback. Please let me know if the latest change addressed your comments.
@hdamker I believe that everything to date has been resolved. 🤞
As discussed: @sfnuser @eric-murray @jlurien please do the final check (and) if possible approval until June 6th. Feel free to create issues for later improvements of the descriptions - the goal is to get the PR merged if possible asap.
@hdamker It's difficult to review the overall API definition file because Randy's fork is some 80 commits behind the main branch. Whilst much of this is other files, there have been some changes qod-api.yaml that are not included in Randy's PR.
@RandyLevensalor Can you sync your fork with the main branch (and fix any conflicts that may arise)? This has to be done sometime, so might as well do it now.
@hdamker It's difficult to review the overall API definition file because Randy's fork is some 80 commits behind the main branch. Whilst much of this is other files, there have been some changes qod-api.yaml that are not included in Randy's PR.
@RandyLevensalor Can you sync your fork with the main branch (and fix any conflicts that may arise)? This has to be done sometime, so might as well do it now.
@eric-murray I've rebased the branch.
Surprisingly enough, there were no conflicts and I didn't see any regressions. Though an additional set of eyes on this will make me feel better.
@jlurien Thanks for the additional suggestions. I've merged them into the PR.
LGTM - thanks @sfnuser @jlurien @eric-murray for the detailed reviews and your approvals. I will try to delete the patchfile.diff and merge.
Implememnted issue #125