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

GH-79 Copy and delete performance #80

Closed
wants to merge 1 commit into from
Closed

GH-79 Copy and delete performance #80

wants to merge 1 commit into from

Conversation

auniverseaway
Copy link
Member

  • Copy and delete now share a listCommand - DRY
  • Copy and delete now pass down a continuationToken
  • Delete will now look for formData

Resolves: GH-79

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

* Copy and delete now share a listCommand - DRY
* Copy and delete now pass down a continuationToken
* Delete will now look for formData

Resolves: GH-79
@auniverseaway auniverseaway requested a review from bstopp September 8, 2024 04:01
Copy link
Contributor

@bstopp bstopp left a comment

Choose a reason for hiding this comment

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

I'll be honest: Not a fan of exposing the use of the continuation token to the client so that it can continue to make future requests for move/copy.

I think it'd make more sense that if there will be a truncated list, create an offline/async process that the client can check/monitor. If not truncated, just perform the op.

@bstopp
Copy link
Contributor

bstopp commented Sep 10, 2024

Also, on a delete operation, there's no need for the continuation token. After the first pass of the list command, those files which were processed will no longer be returned for processing. Could just as easily change the response code from 200 => 204, or another successful response value which indicates there are no more entries to process.

If the rename operation did not allow something to move to a child, the same would be true about that.

Since rename does allow a folder to move to a descendant, I'm not entirely convinced this wouldn't create an infinite recursion even with the continuation token....

@auniverseaway
Copy link
Member Author

I think it'd make more sense that if there will be a truncated list, create an offline/async process that the client can check/monitor. If not truncated, just perform the op.

I agree with this sentiment, but the amount of work involved in doing this correctly is substantial. We have cases today where jmp is blocked and running into errors due to this bug.

@auniverseaway
Copy link
Member Author

Also, on a delete operation, there's no need for the continuation token. After the first pass of the list command, those files which were processed will no longer be returned for processing. Could just as easily change the response code from 200 => 204, or another successful response value which indicates there are no more entries to process.

If the rename operation did not allow something to move to a child, the same would be true about that.

Since rename does allow a folder to move to a descendant, I'm not entirely convinced this wouldn't create an infinite recursion even with the continuation token....

This makes sense. I'll adjust.

@bstopp
Copy link
Contributor

bstopp commented Sep 25, 2024

I thought about this some more, and I don't think the continuation token is context-aware enough to prevent an infinite recursion during a copy operation, where the destination is a descendant of the source

@auniverseaway
Copy link
Member Author

auniverseaway commented Sep 26, 2024

where the destination is a descendant of the source

This makes sense. I think we have a couple things we can do on this front. One of them is to get the full list up front and deliver back down to the client what wasn't finished... this might actually be the best way to handle this use case for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve copy and delete performance
2 participants