Skip to content
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

[Communication Services - JobRouter] REST API Review #18013

Closed
cparisineti opened this issue Feb 25, 2022 · 5 comments · Fixed by #17872 or #23804
Closed

[Communication Services - JobRouter] REST API Review #18013

cparisineti opened this issue Feb 25, 2022 · 5 comments · Fixed by #17872 or #23804
Assignees
Labels
API Review Scoping This is an issue that will track work on a specific set of API changes. APIStewardship-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review

Comments

@cparisineti
Copy link
Contributor

  • Service name: ACS Job Router
  • Key contact for the review: Bo Gao (bga@microsoft.com)
  • Whether this is a new or existing API - New API
  • Whether or not your service already been deployed - yes
  • A brief description of the material to be reviewed/changed - APIs (swagger model) has been reviewed with Jeffrey Richter. This PR includes swagger definition and examples for JobRouter.
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 25, 2022
@cparisineti cparisineti added the APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. label Feb 25, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Feb 25, 2022
@cparisineti cparisineti added the API Review Scoping This is an issue that will track work on a specific set of API changes. label Feb 25, 2022
@cparisineti
Copy link
Contributor Author

Pull request: #17872

@cparisineti cparisineti linked a pull request Feb 28, 2022 that will close this issue
1 task
@czubair czubair changed the title [Communication - JobRouter] REST API Review [Communication Services - JobRouter] REST API Review Feb 28, 2022
@markweitzel
Copy link
Member

markweitzel commented Mar 8, 2022

API Stewardship Board Review Meeting 8-March-22

Discussion / Notes

  • Use PATCH or PUT for creation. The id will be classified as OII data and not classified as customer content. Even if it was classified as customer content, this is still allowed.

  • queueSelectors and workerSelectors

    • Order in the array does not matter
    • These are only primitive values. They are and'ed together
  • For GET routing/jobs

    • IF there are more sophisticated filter conditions that are necessary, consider adding a more robust filter language instead of and'ing the two properties.
  • For TTL: We are encouraging customers to move away from ISO8601

    • Consider not using ISO8601 durations in the PXX format because there's not great support or understanding (on things like selector's ttl). Instead pick a granularity (seconds, minutes, hours, ?) and allow people to define the number of those units for ttl.
    • This should apply to all your durations

Actions

  • Sync with @JeffreyRichter off-line on privacy concerns
  • agfood is an example of an OpenAPI document with a default response.

Next Steps:

  • Clean up the small issues & when complete, we'll do an async review.
  • Not gated for private preview.
  • Make sure to schedule another review session prior to GA or if some other issues arise.

References

Meeting Chat
Meeting Recording
Transcript

Helpful Documentation

@markweitzel markweitzel added APIStewardship-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review and removed APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. labels Mar 8, 2022
@markweitzel
Copy link
Member

Breaking Changes Review Board

Following up on action items from today’s breaking changes office hours for ACS JobRouter:-
New version has been created: 2022-07-18-preview
Changes in new version:-
OperationId change for ops w.r.t., classification policy, exception policy, distribution policy and queue. Reason: Feedback from SDK review -> From SDK pov, we are splitting up the clients into 2 parts – for admin functions and day-to-day functions
Change in model definition:-
WaitTimeExceptionTrigger. Property “threshold” renamed to “thresholdSeconds”. Reason: To correctly represent server-side data type change from TimeSpan to double.
JobQueue. Property “distributionPolicyId” has been removed as required field. Reason: Bugfix.
Model Renames (review feedback after SDK Review):-
All “XXXXXResponse” -> “XXXXXResult”
All “PagedXXXXXX” -> “XXXXXItem”
“AzureFunctionRule” -> “FunctionRule”
Add suffix “Attachment” to all derived classes of “WorkerSelectorAttachment” and “QueueSelectorAttachment”
Enum “JobStatus” -> “RouterJobStatus”

Link to PR: [Azure.Communication.Services][JobRouter] Modify swagger after SDK review by sarkar-rajarshi · Pull Request #19750 · Azure/azure-rest-api-specs (github.com)
Link to ADO Workitem: Scenario 14958963: [ACS][JobRouter] Breaking change review - Boards (visualstudio.com)

@azure-sdk
Copy link
Collaborator

New API Review meeting has been requested.

Service Name: IC3 ACS CCaaP
Review Created By: Rajarshi Sarkar
Review Date: 05/30/2023 01:00 PM PT
Onboarding Record:
PR: #23804
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description: Re-review of previous version with additional modifications for new features (all additive changes):-

  • Worker configuration can be modified to limit maximum number of jobs for a given channel

  • Job can be scheduled to be queued at a future time

  • New RouterRule type: WebhookRule

  • During job unassignment, job matching can be paused if required

Detailed meeting information and documents provided can be accessed here
For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

@mikekistler
Copy link
Member

Notes from REST API Review 5/30/23

  • What are the competitors to this service?
  • maxpagesize should be all lowercase
  • limit consumes to "application/json"
  • "unavailableForMatching" seems like it is a double-negative, maybe because the default is false?
    • maybe change this from boolean to an enum? That will fix the double-negative naming and allow more options to be expressed.
    • maybe a polymorphic type: JobMatchingMode
  • Could "waitForActivation" be a query parameter rather than in the request body?
  • Why "modelAsString: false"? We recommend "modelAsString: true"
  • JobStatus values should use our LRO terminal state values, even if this isn't a "true" LRO
  • Could "reofferTimeUTC" be a query parameter?
  • Could you remove "UTC" from the names of date-time values?

This looks good for preview after clean up of the minor issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Review Scoping This is an issue that will track work on a specific set of API changes. APIStewardship-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review
Projects
Status: Done
5 participants