Conversation
add api yaml
hdamker
left a comment
There was a problem hiding this comment.
Please see my suggestion for compliance with the new API versioning and release process.
|
Thanks Herbert, we'll start the release process. |
@ShutingQing No rush needed, take first the time to have a work in progress version which is discussed within the team and agreed to be (pre-)released. The important point is that this PR here will create a "wip" version but not a version with a version number. Note: I have not commented on the content of the PR. |
Update API version to wip
| ServiceArea: | ||
| $ref: "#/components/schemas/Area" | ||
| SLAProfile: | ||
| $ref: "#/components/schemas/SLAProfile" |
There was a problem hiding this comment.
SLAProfile
I suggest to remove the prefix "SLA".
1: The area and the service time should be part of the SLA as well.
2: An SLA contains may other aspects, which are not handled by the API.
| properties: | ||
| MaxNumofTerminals: | ||
| $ref: "#/components/schemas/NumofTerminals" | ||
| DLThroughputPerTerminal: |
There was a problem hiding this comment.
What is the relation to the QOS Profiles, as developed by QOD Group?
| type: object | ||
| properties: | ||
| min: | ||
| $ref: "#/components/schemas/Float" |
There was a problem hiding this comment.
I suggest to simplify the yaml and use the type number directly. --> data type is number for format float
| maximum: 20 | ||
|
|
||
|
|
||
| Throughput: |
There was a problem hiding this comment.
I suggest to allow unit prefixes like kilo or mega.
| type: object | ||
| properties: | ||
| StartTime: | ||
| $ref: "#/components/schemas/TimeStamp" |
There was a problem hiding this comment.
Do we really need a Data type for TimeStamp? It should be a simple string with a format date-time.
| EndTime: | ||
| $ref: "#/components/schemas/TimeStamp" | ||
|
|
||
| Float: |
There was a problem hiding this comment.
I suggest to remove the data-type and use the OpenAPI "number" data-type. See Section on Numbers
| maximum: 180 | ||
|
|
||
| TimeStamp: | ||
| type: string |
There was a problem hiding this comment.
I suggest to add "format: date-time" (i.e. not just in the description).
| type: array | ||
| items: | ||
| $ref: "#/components/schemas/Point" | ||
| minItems: 3 |
There was a problem hiding this comment.
There should be a requirement, that the third point is not on the same line as the first two points.
There was a problem hiding this comment.
As this area definition is used also in other APIs I suggest to bring the discussion to DeviceLocation and/or Commonalities. As a minimum requirement the polygon must not be self-intersecting.
There was a problem hiding this comment.
Aha, yes, would be good to re-use data types (when possible). Likely the best to bring this discussion to Commonalities.
I opened an issue in commonalities (Issue 242)
Requested formal changes done, but I haven't reviewed the content, hence can't approve it.
tlohmar
left a comment
There was a problem hiding this comment.
Some updates, specifically around the Area data type.
| description: Number of terminals | ||
| format: int32 | ||
| minimum: 1 | ||
| maximum: 20 |
There was a problem hiding this comment.
Do you need to limit range with the maximum number of terminals which is 20? I would like to suggest to remove "maximum: 20".
There was a problem hiding this comment.
"Maximum Num of Terminals" is suggested to keep it. This is an attribute that used to evaluation whether Network can assure the slice to be booked and meet the customers' need.
There was a problem hiding this comment.
To keep the parameter "Maximum Num of Terminal" itself is fine with me. I wonder the reason to limit the number 20 (and not 30 or 50)?
| @@ -0,0 +1,293 @@ | |||
| openapi: 3.0.3 | |||
| info: | |||
| title: API for Network slicing Booking | |||
There was a problem hiding this comment.
-> "API for Network Slice Booking"
| schema: | ||
| $ref: "#/components/schemas/SessionId" | ||
| responses: | ||
| "204": |
There was a problem hiding this comment.
i believe we need some error code here, so as get session
| type: integer | ||
| example: 10 | ||
| description: | | ||
| An attribute specifies the required DL packet transmission latency (millisecond) |
There was a problem hiding this comment.
We can use a common rate data type here.
ShutingQing
left a comment
There was a problem hiding this comment.
I left some comments, please check files changed. Thank you. : )
| properties: | ||
| MaxNumofTerminals: | ||
| $ref: "#/components/schemas/NumofTerminals" | ||
| DLThroughputPerTerminal: |
There was a problem hiding this comment.
Each terminal should have different DL UP throughput.
| NumofTerminals: | ||
| type: integer | ||
| example: 5 | ||
| description: Number of terminals |
There was a problem hiding this comment.
Add some descriptions.
There was a problem hiding this comment.
"Maximum Num of Terminals" is suggested to keep it. This is an attribute that used to evaluation whether Network can assure the slice to be booked and meet the customers' need.
| description: Number of terminals | ||
| format: int32 | ||
| minimum: 1 | ||
| maximum: 20 |
There was a problem hiding this comment.
One more thing. Just let me ask you.
Device access management is mentioned in api design #20.
In this PR, the API can specify the number of terminals but cannot specify which terminals will be allowed to access the slice.
Do you have a plan to add more parameter related to device access management or create new device access management API later?
There was a problem hiding this comment.
Yes Masa, based on Thorsten's suggestion, we'd better need to make device access management function independently. Basically there is one slice resource reservation function, and another device access management function.
| $ref: "#/components/responses/Generic503" | ||
| get: | ||
| tags: | ||
| - NSB Sessions |
There was a problem hiding this comment.
| - NSB Sessions | |
| - Network Slice Booking Sessions |
There was a problem hiding this comment.
@ShutingQing you need to commit this suggestion as well, otherwise it doesn't fit to line 17
|
Thanks so much Herbert for the detailed suggestions attached. We'll look into it. |
|
Hi all. Based on the last meeting discussion, there were no further comments. Later for Herbert's suggestions, we've modified one version. Currently the version looks good to me. Please other reviewers take a look. If there is no further comments, we give this an approve. |
tlohmar
left a comment
There was a problem hiding this comment.
Would be good to update the PullRequest and implement the comments. It is now a bit difficult to judge, how the various comments will modify the yaml.
| NumberOfTerminals: | ||
| type: integer | ||
| example: 5 | ||
| description: Number of terminals |
There was a problem hiding this comment.
The term 'device' is used in other CAMARA APIs and I assume, that a 'terminal" is the same as a 'device'. When there is indeed no difference between 'terminal' and 'device', I suggest to use change to 'MaxNumberOfDevices' (or MaximumNumberOfDevices)..
There was a problem hiding this comment.
The term 'device' is used in other CAMARA APIs and I assume, that a 'terminal" is the same as a 'device'. When there is indeed no difference between 'terminal' and 'device', I suggest to use change to 'MaxNumberOfDevices' (or MaximumNumberOfDevices)..
That's okay for me. Fan and me will modify this. For further comments, I'll suggest to initiate another issue and pr to keep updating the yaml.
There was a problem hiding this comment.
I suppose the YAML within this PR is now a good "work in progress" starting point for the further work within the repository and I recommend to merge it as soon as possible to allow to create issues for the further discussion needed.
... For further comments, I'll suggest to initiate another issue and pr to keep updating the yaml.
That can be done only after there is a yaml merged into main. Currently the base of the discussion is only this PR here.
And thanks for accepting my suggestions - I resolved the conversations. There was one overseen which needs to be committed as there is now no "NSB Sessions" tags defined anymore.
One point which you might want to consider already here or open as an issue (as it might need some discussion):
- The schema which is now named as
QoSProfileis something different than the resource definitionQoSProfilewithin the qos-profile API in QualityOnDemand. I would also say that it isn't a QoS Profile (for a single device), but the parameters for the requested slice (see e.g. the parameterMaxNumofTerminals). So something like "SliceProfile" maybe?
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
add api yaml v1