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

Jump to bottom

[TaskManager] Add APIs to mark tasks as complete #11649

Merged
merged 21 commits into from Sep 6, 2022
Merged

[TaskManager] Add APIs to mark tasks as complete #11649

merged 21 commits into from Sep 6, 2022

Conversation

scottn12
Copy link
Contributor

@scottn12 scottn12 commented Aug 24, 2022 •
edited by azure-boards bot

Description

This PR adds the necessary APIs/events to mark tasks as complete. Marking a task as complete will automatically remove all clients from the queue and fire the completed event.

This PR also reorganizes the unit tests.

Reviewer Guidance

Would be great to receive feedback on:

  • Thoughts on being able to re-volunteer for the same taskId after it was completed (currently allowed in this iteration).
  • API naming - specifically markAsCompleted() vs complete()
  • New unit test organization
  • Ideas for an example to help demonstrate this API

Other information or known dependencies

AB#1545
MSFT Internal Design Doc

@github-actions github-actions bot added area: dds Issues related to distributed data structures public api change Changes to a public API base: next PRs targeted against next branch labels Aug 24, 2022
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Aug 24, 2022 •
edited

Could not find a usable baseline build with search starting at CI 179502b

Generated by 🚫 dangerJS against 007f44c

@scottn12 scottn12 marked this pull request as ready for review August 25, 2022 17:39
@scottn12 scottn12 requested review from msfluid-bot and a team as code owners August 25, 2022 17:39
ChumpChief
ChumpChief reviewed Aug 26, 2022
View reviewed changes
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

Looks pretty much good, though I think the eventing might need some further thought. I think it's important for the eventing to happen when visible changes to the imperative API surface happen.

api-report/task-manager.api.md Outdated Show resolved Hide resolved
packages/dds/task-manager/src/interfaces.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Show resolved Hide resolved
packages/dds/task-manager/src/test/taskManager.spec.ts Outdated Show resolved Hide resolved
@scottn12 scottn12 mentioned this pull request Aug 30, 2022
Merged
msftbot bot pushed a commit that referenced this pull request Aug 31, 2022
@scottn12
[TaskManager DDS] Reorganize test file (#11727)
29e99b3 
This PR reorganizes the UT file. This is done is a separate PR to reduce the number of changes in #11649.
ChumpChief
ChumpChief reviewed Aug 31, 2022
View reviewed changes
packages/dds/task-manager/src/interfaces.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/test/taskManager.spec.ts Show resolved Hide resolved
packages/dds/task-manager/src/test/taskManager.spec.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/test/taskManager.spec.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/test/taskManager.spec.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added area: runtime Runtime related issues area: server Server related issues (routerlicious) area: tests Tests to add, test infrastructure improvements, etc dependencies Pull requests that update a dependency file labels Sep 6, 2022
@github-actions github-actions bot removed area: dds: sharedstring area: driver Driver related issues area: build Build related issues area: examples Changes that focus on our examples area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc dependencies Pull requests that update a dependency file area: server Server related issues (routerlicious) labels Sep 6, 2022
@scottn12 scottn12 requested review from a team and removed request for a team September 6, 2022 15:11
@msftbot
Copy link
Contributor

msftbot bot commented Sep 6, 2022

Hello @scottn12!

Because this pull request has the msftbot: merge-next label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@msftbot msftbot bot merged commit 80fe63d into microsoft:next Sep 6, 2022
@scottn12 scottn12 deleted the addCompletedAPIs branch September 6, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChumpChief ChumpChief ChumpChief approved these changes

@ssimic2 ssimic2 Awaiting requested review from ssimic2

@msfluid-bot msfluid-bot Awaiting requested review from msfluid-bot

Assignees
No one assigned
Labels
area: dds Issues related to distributed data structures base: next PRs targeted against next branch msftbot: merge-next public api change Changes to a public API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@scottn12 @msfluid-bot @ChumpChief