Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Introduce 'qosStatus' and corresponding notification event to fix issue #38 #67

Merged
merged 23 commits into from
Feb 9, 2023
Merged

Introduce 'qosStatus' and corresponding notification event to fix issue #38 #67

merged 23 commits into from
Feb 9, 2023

Conversation

emil-cheung
Copy link
Collaborator

A proposal to fix:
#38

@emil-cheung emil-cheung closed this Nov 7, 2022
@emil-cheung emil-cheung reopened this Nov 7, 2022
akoshunyadi
akoshunyadi reviewed Nov 9, 2022
View reviewed changes
patrice-conil
patrice-conil previously approved these changes Nov 9, 2022
View reviewed changes
jlurien
jlurien reviewed Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand the purpose of this new model after reading issues #33 and #38. The purpose is to allow an asynchronous model for cases when it is not possible to fulfill the request for createSession synchronously?

@jlurien jlurien mentioned this pull request Nov 11, 2022
Closed
@eric-murray eric-murray mentioned this pull request Nov 17, 2022
Closed
@emil-cheung
Copy link
Collaborator Author

emil-cheung commented Nov 21, 2022 •
edited
Loading

The deck I presented in the WG meeting associated to this PR:
QoS Status and Corresponding Notification Events

jlurien
jlurien reviewed Dec 2, 2022
View reviewed changes
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @emil-cheung for addressing past comments. Now that I understand better your proposal I could review it with more context.

emil-cheung and others added 4 commits December 3, 2022 14:08
@emil-cheung @jlurien
Update code/API_definitions/qod-api.yaml
2c7f022 
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@emil-cheung @jlurien
Update code/API_definitions/qod-api.yaml
1b659f1 
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@emil-cheung @jlurien
Update code/API_definitions/qod-api.yaml
682a48b 
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@emil-cheung @jlurien
Update code/API_definitions/qod-api.yaml
7d96073 
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@hdamker hdamker mentioned this pull request Jan 17, 2023
Closed
@emil-cheung
Copy link
Collaborator Author

@hdamker shall I rebase this PR to the main branch?

@hdamker
Copy link
Collaborator

hdamker commented Feb 7, 2023

shall I rebase this PR to the main branch?

@emil-cheung sorry for the late answer (thought I had done it already): yes, please rebase the PR to main branch as this is now the branch for the latest changes.

@emil-cheung emil-cheung changed the base branch from dev-v0.9.0 to main February 8, 2023 07:08
@emil-cheung emil-cheung dismissed stale reviews from eric-murray and patrice-conil February 8, 2023 07:08

The base branch was changed.

@emil-cheung
Copy link
Collaborator Author

shall I rebase this PR to the main branch?

@emil-cheung sorry for the late answer (thought I had done it already): yes, please rebase the PR to main branch as this is now the branch for the latest changes.

@hdamker I have changed the base branch from dev-0.9.0 to main, please check.

@hdamker
Copy link
Collaborator

hdamker commented Feb 9, 2023

I have changed the base branch from dev-0.9.0 to main, please check.

@emil-cheung Thanks a lot! LGTM, we are ready for a final review by those who have previously approved.

hdamker
hdamker previously approved these changes Feb 9, 2023
View reviewed changes
@hdamker
Copy link
Collaborator

hdamker commented Feb 9, 2023

@jlurien @jlurien @eric-murray Could you have a final view?

jlurien
jlurien reviewed Feb 9, 2023
View reviewed changes
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A couple of cosmetic suggestions

sfnuser
sfnuser previously approved these changes Feb 9, 2023
View reviewed changes
@emil-cheung @jlurien
Update code/API_definitions/qod-api.yaml
df530ca 
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
@emil-cheung emil-cheung dismissed stale reviews from sfnuser and hdamker via df530ca February 9, 2023 10:31
sfnuser
sfnuser reviewed Feb 9, 2023
View reviewed changes
code/API_definitions/qod-api.yaml Outdated
@@ -397,7 +397,10 @@ components:
enum:
- DURATION_EXPIRED
- NETWORK_TERMINATED
description: Currently statusInfo is only applicable when qosStatus is 'UNAVAILABLE'.
description: |
Reason for the new `qosStatus`. Currently `statusInfo` is only applicable when `osStatus` is 'UNAVAILABLE'.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

osStatus -> qosStatus

@emil-cheung @jlurien
Update code/API_definitions/qod-api.yaml
fb1c5d6 
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
eric-murray
eric-murray reviewed Feb 9, 2023
View reviewed changes
code/API_definitions/qod-api.yaml Outdated
@@ -397,7 +397,10 @@ components:
enum:
- DURATION_EXPIRED
- NETWORK_TERMINATED
description: Currently statusInfo is only applicable when qosStatus is 'UNAVAILABLE'.
description: |
Reason for the new `qosStatus`. Currently `statusInfo` is only applicable when `osStatus` is 'UNAVAILABLE'.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: `osStatus` should be `qosStatus`

jlurien
jlurien approved these changes Feb 9, 2023
View reviewed changes
Copy link
Collaborator

@jlurien jlurien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hdamker hdamker merged commit d25744f into camaraproject:main Feb 9, 2023
@hdamker hdamker mentioned this pull request Feb 10, 2023
Closed
@emil-cheung emil-cheung deleted the emil-cheung-qos-status-and-notification-events branch June 6, 2023 06:09
@hdamker hdamker mentioned this pull request Jun 17, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bigludo7 bigludo7 bigludo7 left review comments

@akoshunyadi akoshunyadi akoshunyadi left review comments

@jlurien jlurien jlurien approved these changes

@hdamker hdamker hdamker approved these changes

@eric-murray eric-murray eric-murray approved these changes

@sfnuser sfnuser sfnuser approved these changes

@jpengar jpengar Awaiting requested review from jpengar

@SyeddR SyeddR Awaiting requested review from SyeddR

@SylMOREL SylMOREL Awaiting requested review from SylMOREL

@ToshiWakayama-KDDI ToshiWakayama-KDDI Awaiting requested review from ToshiWakayama-KDDI

@shilpa-padgaonkar shilpa-padgaonkar Awaiting requested review from shilpa-padgaonkar

@patrice-conil patrice-conil Awaiting requested review from patrice-conil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@emil-cheung @jpengar @hdamker @jlurien @patrice-conil @bigludo7 @akoshunyadi @eric-murray @sfnuser