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

Rework TaskManager task locking APIs #11530

Merged
merged 24 commits into from Aug 24, 2022
Merged

Conversation

scottn12
Copy link
Contributor

@scottn12 scottn12 commented Aug 12, 2022

Description

This PR reworks TaskManager.lockTask() to volunteerForTask() and introduces TaskManager.subscribeToTask().

volunteerForTask()

This API largely remains unchanged from lockTask(). One major difference is that the return type was changed form Promise<void> to Promise<boolean>. This was done to prepare for the API eventually return false in certain circumstances, mainly if the task has already been completed (see internal design doc linked below).

The main motivation behind the rename was to be more similar to subscribeToTask() and to emphasize that there is no guarantee that this API will ever obtain the task lock.

subscribeToTask()

This API is similar to volunteerForTask() with a few key differences. First, this API will continuously try to lock the given task. If the lock is lost due to losing connection, then we will automatically try to re-enter the queue upon reconnection. This is ideal for ongoing or asynchrounous tasks that do not have a definitive end. Second, this API does not have a return value, since calling this API once can result in the task lock being assigned and lost multiple times. To effectively use this API, the caller must rely on listening to the assigned and lost events to determine if the task lock is assigned to the client.

subscribed()

This API simply returns a boolean to indicate if the client is currently subscribed to a given task.

haveTaskLock() / assignedTask()

This PR also renames haveTaskLock() to assignedTask(). This was done to be more consistent with the assigned event, as well as the new APIs. There are no functional changes to this API.

Clicker Demo

This PR also updates the clicker demo to demonstrate a case where subscribeToTask() is useful and how someone may implement it in their application. The previous iteration of clicker demo would always try obtaining the task lock no matter what. If the lock was lost the client would simply re-enter the queue if possible. This is a good opportunity to demonstrate subscribeToTask() and the coding patterns associated with it.

Reviewer Guidance

Would appreciate feedback on:

  • New API names
  • New coding patterns
  • Implementation details
  • Return values of the above functions

Other information or known dependencies

MSFT Internal Design Doc

AB#1466

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: examples Changes that focus on our examples area: tests Tests to add, test infrastructure improvements, etc breaking change This PR or issue would introduce a breaking change public api change Changes to a public API base: next PRs targeted against next branch labels Aug 12, 2022
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Aug 12, 2022

@fluid-example/bundle-size-tests: +15.8 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 392.45 KB 394.18 KB +1.74 KB
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 191.92 KB 197.48 KB +5.55 KB
loader.js 151.12 KB 151.06 KB -57 Bytes
map.js 42.63 KB 47.38 KB +4.75 KB
matrix.js 131.63 KB 134.39 KB +2.77 KB
odspDriver.js 150.23 KB 150.11 KB -127 Bytes
odspPrefetchSnapshot.js 38.39 KB 38.35 KB -41 Bytes
sharedString.js 152.42 KB 153.64 KB +1.21 KB
Total Size 1.25 MB 1.27 MB +15.8 KB

Baseline commit: fe10534

Generated by 🚫 dangerJS against 830a55b

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.

Exciting! :-D

api-report/task-manager.api.md Outdated Show resolved Hide resolved
examples/data-objects/clicker/src/index.tsx Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts 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 Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Show resolved Hide resolved
@scottn12 scottn12 marked this pull request as ready for review August 16, 2022 20:20
@scottn12 scottn12 requested review from msfluid-bot and a team as code owners August 16, 2022 20:20
examples/data-objects/clicker/src/index.tsx Outdated Show resolved Hide resolved
examples/data-objects/clicker/src/index.tsx Outdated Show resolved Hide resolved
packages/dds/task-manager/src/test/taskManager.spec.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/interfaces.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
packages/dds/task-manager/src/taskManager.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Show resolved Hide resolved
@skylerjokiel
Copy link
Contributor

@scottn12 , really like the additional thinking and direction here. I don't think I fully get the nuances of the new API.

Some questions to help clarify:

  • What's the difference between assigned and subscribed?
  • Given the volunteerForTask, how would one go about abandoning a the task in a way everyone would know it's completed?
  • I feel that subscribeToTask is just a subset of volunteerForTask where no one ever marks the task as completed. Why does this need to be a separate api?

@scottn12
Copy link
Contributor Author

@skylerjokiel, I'd be happy to help clarify!

  • What's the difference between assigned and subscribed?

Assigned means that you are the client who currently has the task lock and is allowed to execute a specific task with the guarantee that no other client is also executing the task. Being subscribed to a task means that you will always be trying to obtain the task lock whenever possible (by entering the queue).

  • Given the volunteerForTask, how would one go about abandoning a the task in a way everyone would know it's completed?

This is something I am currently saving for my next PR. If you take a look at the internal design doc (linked in description), you will see a markAsComplete() API that I am planning to implement, along with a new completed event. Currently, you can call abandon() when you reach a certain completion criteria, but this will just pass the task lock down to all the remaining clients in queue as they also abandon the task one by one until no clients are left in queue. My plan is to implement markAsComplete() in a way where all clients will automatically be removed from the queue upon completion of the task.

  • I feel that subscribeToTask is just a subset of volunteerForTask where no one ever marks the task as completed. Why does this need to be a separate api?

Currently neither API can really mark a task as complete (see above). I made this a separate API for scenarios where we will want to have clients continuously attempt to volunteer for a task whenever it's possible, such as ongoing and asynchronous tasks. In cases like these, we can avoid calling volunteerForTask() multiple times and call subscribeToTask() once. I modified the clicker example in this PR to help demonstrate the new coding patterns.

@scottn12 scottn12 requested a review from ssimic2 August 19, 2022 17:12
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.

Little more feedback, mostly interested in the abandon() method

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/test/test-service-load/src/loadTestDataStore.ts Outdated Show resolved Hide resolved
packages/dds/task-manager/src/taskManager.ts Show resolved Hide resolved
Co-authored-by: Matt Rakow <ChumpChief@users.noreply.github.com>
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.

Nice work -- looks good to me! :)

@msftbot
Copy link
Contributor

msftbot bot commented Aug 24, 2022

Hello @sonalivdeshpande!

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 7225ebd into microsoft:next Aug 24, 2022
@scottn12 scottn12 deleted the newTaskManagerAPIs branch August 24, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds Issues related to distributed data structures area: examples Changes that focus on our examples area: tests Tests to add, test infrastructure improvements, etc base: next PRs targeted against next branch breaking change This PR or issue would introduce a breaking change msftbot: merge-next public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants