feat(copy): introduces CopyGraphOptions with events support#145
feat(copy): introduces CopyGraphOptions with events support#145leonardochaia wants to merge 33 commits intooras-project:mainfrom
Conversation
leonardochaia
changed the title
…ries Signed-off-by: Leonardo Chaia <[email protected]>
leonardochaia
force-pushed
the
feat/improve-copy-performance
branch
from
847ecb0 to
6817504
Compare
Signed-off-by: Leonardo Chaia <[email protected]>
|
This is a work in progress. Seems to be working running locally. Tests are failing and need more work. |
Signed-off-by: Leonardo Chaia <[email protected]>
Signed-off-by: Leonardo Chaia <[email protected]>
leonardochaia
requested review from
Wwwsylvia,
sammychinedu2ky and
shizhMSFT
as code owners
|
Hi. I propose concurrency support is implemented in a separate PR. Otherwise this should be ready to review :+ Leo. |
|
Example usage: List<string> knownReferences = [];
foreach (var toReplicate in toReplicateArray)
{
var copyOpts = new CopyOptions()
{
MountFrom = _ => knownReferences.ToArray(),
};
copyOpts.OnCopySkipped += d => this.logger.LogTrace("{Digest}: Already exists.", d.Digest);
copyOpts.OnPreCopy += d => this.logger.LogTrace("{Digest}: Copying..", d.Digest);
copyOpts.OnPostCopy += d => this.logger.LogTrace("{Digest}: Copied", d.Digest);
copyOpts.OnMounted += (d, from) => this.logger.LogTrace("{Digest}: Mounted from {From}", d.Digest, from);
await sourceRepository.CopyAsync(
toReplicate.SourceImage,
destinationRepository,
toReplicate.DestinationImage,
cancellationToken,
copyOpts);
knownReferences.Add(toReplicate.DestinationImage);
} |
|
Hi all, any feedback is appreciated 🥇 Thanks! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
+ Coverage 89.06% 89.25% +0.19%
==========================================
Files 39 41 +2
Lines 1591 1620 +29
Branches 204 211 +7
==========================================
+ Hits 1417 1446 +29
+ Misses 113 112 -1
- Partials 61 62 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @leonardochaia , thank you for your contribution! Sorry for the delay. We need a bit more time to review your PR, but we’ll provide feedback as soon as we can. Thank you for your understanding! |
Do we want to move the mounting support to a separate PR as well? |
Hi @Wwwsylvia , thanks for the review. I'll apply the changes and report back. My goal is getting the mounting support when copying graphs since we are using this to replicate a bunch of images between registries and need to make it faster. |
|
@leonardochaia Sounds good. Thank you! |
Signed-off-by: Leonardo Chaia <[email protected]>
Signed-off-by: Leonardo Chaia <[email protected]>
|
PR simplified to only introduce the CopyGraphOptions. Next step is to update the tests to cover the events. Separate PRs/issues to be created:
|
|
Usually CancellationToken is left as the last parameter, however, that would introduce a breaking change. Are we good with introducing a breaking change? EDIT: Disregard, I've added an overload to keep the CancellationToken last but also support the current signature |
Signed-off-by: Leonardo Chaia <[email protected]>
leonardochaia
changed the title
Signed-off-by: Leonardo Chaia <[email protected]>
|
Quick update, I've converted this PR to only include the Next steps:
|
…us event handlers. Signed-off-by: Leonardo Chaia <[email protected]>
Signed-off-by: Leonardo Chaia <[email protected]>
Signed-off-by: Leonardo Chaia <[email protected]>
|
Hi @leonardochaia , happy new year! Would you like to continue our discussion on this? |
|
We can merge this first and iterate it later. |
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
|
I will make changes based on this branch. |
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces enhancements to the copy functionality by adding CopyGraphOptions with event support for both synchronous and asynchronous operations. Key changes include:
- Extending CopyAsync and CopyGraphAsync to support new CopyOptions and CopyGraphOptions (with events hooks).
- Adding new structs (CopyOptions and CopyGraphOptions) and a helper extension (AsyncInvocationExtensions) to safely invoke asynchronous event delegates.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/OrasProject.Oras.Tests/CopyTest.cs | Added multiple tests to validate copy operations with event hooks and platform filtering. |
| src/OrasProject.Oras/Extensions.cs | Modified CopyAsync and CopyGraphAsync signatures to accept CopyOptions and CopyGraphOptions. |
| src/OrasProject.Oras/CopyOptions.cs | Introduced CopyOptions struct to encapsulate CopyGraphOptions. |
| src/OrasProject.Oras/CopyGraphOptions.cs | Added CopyGraphOptions struct containing pre/post copy and skipped events. |
| src/OrasProject.Oras/AsyncInvocationExtensions.cs | Provided an extension for invoking asynchronous event delegates using new C# syntax. |
Comments suppressed due to low confidence (1)
tests/OrasProject.Oras.Tests/CopyTest.cs:153
- [nitpick] Consider renaming the test 'TestCopyExistedRoot' to 'TestCopyExistingRoot' for improved clarity.
[Fact] public async Task TestCopyExistedRoot()
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces event-driven graph copy options, enhancing the CopyAsync and CopyGraphAsync APIs with pre/post copy and skip hooks, and adds supporting types and tests.
- Added
CopyOptionsandCopyGraphOptionsstructs with event delegates. - Updated
Extensions.CopyAsyncandCopyGraphAsyncto invoke the new event hooks. - Introduced
AsyncInvocationExtensionsfor safe parallel invocation of async event handlers. - Expanded unit tests in
CopyTest.csto cover skip, pre-copy, and post-copy events, and tracking behavior.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/OrasProject.Oras.Tests/CopyTest.cs | Added tests for skip, pre-copy, and post-copy event handlers; implemented StorageTracker. |
| src/OrasProject.Oras/Extensions.cs | Overloaded CopyAsync/CopyGraphAsync to accept options and trigger CopyGraphOptions events. |
| src/OrasProject.Oras/CopyOptions.cs | Introduced CopyOptions struct wrapping CopyGraphOptions. |
| src/OrasProject.Oras/CopyGraphOptions.cs | Defined CopyGraphOptions with sync/async event delegates and internal invocation methods. |
| src/OrasProject.Oras/AsyncInvocationExtensions.cs | Added InvokeAsync extension to safely call async event delegates in parallel. |
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
Signed-off-by: Lixia (Sylvia) Lei <[email protected]>
|
Refactored the code a bit more, please take a look again @leonardochaia @shizhMSFT @pat-pan |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces CopyGraphOptions with event support to improve the copy operations. Key changes include:
- New CopyGraphOptions and CopyOptions classes supporting pre-/post-copy and copy-skipped event hooks.
- Updated Extensions methods to pass and use these options.
- Expanded tests to verify the behavior of copy operations with the new event hooks.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/OrasProject.Oras.Tests/CopyTest.cs | Added tests for full graph copy, copying an existing root, and copy operations using hooks. |
| src/OrasProject.Oras/Extensions.cs | Updated copy method signatures to accept new CopyOptions and CopyGraphOptions, and integrated event hook calls. |
| src/OrasProject.Oras/CopyOptions.cs | Introduced CopyOptions as a subclass of CopyGraphOptions. |
| src/OrasProject.Oras/CopyGraphOptions.cs | Added a new class that facilitates pre-/post-copy and copy-skipped events with asynchronous support. |
| src/OrasProject.Oras/AsyncInvocationExtensions.cs | Added extension methods to safely invoke event delegates asynchronously. |
| public int PushCount { get; private set; } | ||
| public int ExistsCount { get; private set; } | ||
|
|
||
| public IList<string> Fetched { get; } = []; |
There was a problem hiding this comment.
The property 'Fetched' is initialized with an empty array literal, which results in an immutable collection. Consider initializing it with a modifiable collection such as 'new List()' to allow dynamic additions.
| public IList<string> Fetched { get; } = []; | |
| public IList<string> Fetched { get; } = new List<string>(); |
| return eventDelegate | ||
| .GetInvocationList() | ||
| .Select(d => d is Func<TEventArgs, Task> handler ? handler(args) : Task.CompletedTask) | ||
| .ToArray(); |
There was a problem hiding this comment.
Could the preCopy/postCopy/CopySkip turn into a multicast list?
| /// <summary> | ||
| /// PreCopy handles the current descriptor before it is copied. | ||
| /// </summary> | ||
| public event Action<Descriptor>? PreCopy; |
There was a problem hiding this comment.
just curious about the rationale behind providing this synchronous version of handlers
There was a problem hiding this comment.
It's more convenient to use and users don't have to return Tasks.CompletedTask if they do something synchonously.
See #145 (comment)
| /// <summary> | ||
| /// CopyOptions contains parameters for <see cref="Extensions.CopyAsync(OrasProject.Oras.ITarget,string,OrasProject.Oras.ITarget,string,System.Threading.CancellationToken)"/> | ||
| /// </summary> | ||
| public class CopyOptions : CopyGraphOptions { } |
There was a problem hiding this comment.
Would composition make more sense here? Otherwise, it seems like CopyOptions doesn't do much here. Just wonder why it is needed here
There was a problem hiding this comment.
For now, CopyOptions is just a placeholder. We will add more options into it.
I think inheritance makes it easier for users to initialize and use. They can just do:
var opts = new CopyOptions();
await CopyAsync(src, srcRef, dst, dstRef, opts);
await CopyGraphAsync(src, dst, root, opts);and don't need to do:
var opts = new CopyOptions() {
CopyGraphOptions = new CopyGraphOptions()
};
await CopyAsync(src, srcRef, dst, dstRef, opts);
await CopyGraphAsync(src, dst, root, opts.CopyGraphOptions);
| public event Func<Descriptor, Task>? PreCopyAsync; | ||
|
|
||
| /// <summary> | ||
| /// PreCopy handles the current descriptor before it is copied. | ||
| /// </summary> | ||
| public event Action<Descriptor>? PreCopy; | ||
|
|
||
| /// <summary> | ||
| /// PostCopyAsync handles the current descriptor after it is copied. | ||
| /// </summary> | ||
| public event Func<Descriptor, Task>? PostCopyAsync; | ||
|
|
||
| /// <summary> | ||
| /// PostCopy handles the current descriptor after it is copied. | ||
| /// </summary> | ||
| public event Action<Descriptor>? PostCopy; | ||
|
|
||
| /// <summary> | ||
| /// CopySkippedAsync will be called when the sub-DAG rooted by the current node | ||
| /// is skipped. | ||
| /// </summary> | ||
| public event Func<Descriptor, Task>? CopySkippedAsync; | ||
|
|
||
| /// <summary> | ||
| /// CopySkipped will be called when the sub-DAG rooted by the current node | ||
| /// is skipped. | ||
| /// </summary> | ||
| public event Action<Descriptor>? CopySkipped; |
There was a problem hiding this comment.
Just realized that the event handlers cannot be overwritten, which might not meet our need.
What this PR does / why we need it
This PR improves
CopyAsyncandCopyGraphAsyncto include theCopyGraphOptions, inspired fromoras-go.The current implementation:
CopyOptionsevents, likeOnPreCopy, these were implemented in the form ofAction<T>sPending items
Which issue(s) this PR resolves / fixes
Please check the following list