Access wrapper duplicate type #273
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the ability to require multiple cancel streams in a single job. Requiring is done as you would expect, just target the different streams on the JobConfig. Fulfilling the writer/instance is done by passing the stream instance into the `Fulfill` method. The access wrappers now represent access to the cancel type not a specific cancel stream instance. This works because all of the cancel streams share the same SharedWrite Pending collection so getting write access for one stream grants write access for all. The instance passed through `Fulfill` will get safety checked in the access wrapper and delivered back to be used for writer creation. Behaviour: - One cancel request stream required: `jobData.Fulfill` behaves as normal, no explicit instance required. - N cancel request stream required: `jobData.Fulfill` must be provided the instance for each stream to fulfill otherwise the safety system will throw an exception. Note: This is a workaround implementation. #224 will make this cleaner. This approach can be applied to all write wide (shard write) types. Support will be added in an upcoming commit.
jkeon
reviewed
Jul 31, 2023
...ts/Runtime/Entities/TaskDriver/TaskSet/Job/Wrapper/AbstractDataStreamPendingAccessWrapper.cs
Outdated
Show resolved
Hide resolved
Scripts/Runtime/Entities/TaskDriver/TaskSet/Job/Wrapper/AbstractAccessWrapper.cs
Outdated
Show resolved
Hide resolved
...ts/Runtime/Entities/TaskDriver/TaskSet/Job/Wrapper/AbstractDataStreamPendingAccessWrapper.cs
Outdated
Show resolved
Hide resolved
|
All comments addressed. Ready for re-review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add the ability to require multiple data stream writers of the same type on a task driver job.
The API proposed here provides a solution until a proper solution can be devised during #224
This solution takes advantage of the fact that all data stream pending collections share the same access handle this means that, from a job dependency perspective, getting write access to a stream instance also provides write access to all other stream instances of the same type (shared/wide writing).
What is the current behaviour?
Developers are unable to include writers to multiple data streams of the same type in their job. This most commonly comes up when there are cancel requests to multiple task drivers that a job may want to make during execution.
What is the new behaviour?
Developers can now include multiple data streams of the same type in their job.
jobData.Fulfill()provide the specific stream instance after theoutparameter.How it works
On the first write request we create an access wrapper as we normally would.
For all other write requests after the first one we keep the same access wrapper and track the specific instance request for safety checks. (only if
ANVIL_DEBUG_SAFETYis enabled)During fulfill we use the provided stream instance to generate a writer and make a safety check to ensure that instance was required during job config.
Edge Case Behaviour (When
ANVIL_DEBUG_SAFETYis enabled)Fulfill()when only one instance of the stream type has been requested emits a warning. The defaultFulfill()without the explicit stream should be preferred when possible.What issues does this resolve?
Resolve: #245
What PRs does this depend on?
Does this introduce a breaking change?