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

extract UploadSessionLister interface #4375

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Nov 29, 2023

This is a little cleanup of the UploadProgress interface I had to introduce in #4148

The UploadSessionLister interface allows listing and filtering existing UploadSessions by id, processing or expired state. This can be used by the ocis cli to purge expired AND processed uploads, list uploads that have expired but are still processing etc.

The UploadSession Interface could be extended to include processing steps like the antivirus scan outcome etc, but currently that is stored by the processing service. This may need more thoughts before changing it. I think it makes sense to persist tho outcome of processing steps in the upload session metadata. Unfortunately, we currently store processing stata and scan outcome in the node metadata, not the upload metadata. This will change ofter merging #4148 , then we can think about if and where to persist the processing state ...

For now, I think this PR is ready.

@butonic butonic self-assigned this Nov 29, 2023
Copy link

update-docs bot commented Nov 29, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic butonic requested a review from aduffeck November 29, 2023 19:05
@aduffeck
Copy link
Contributor

The TODOs and FIXMEs should be addressed or turned into issues before merging this in my opinion, but the general interface looks good to me.
How's the filtering of uploads gonna look like though? Does that become part of the ListUploads() call?

@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

I wonder if ListUploads() should take a filter. one by id to replace GetUploadProgress() and one by status to allow implementing cli tooling that allows listing currently ongoing uploads, uploads that are still in postprocessing to see which ones may have stalle etc. Hm, let me check ifg the graph API has anything interesting...

👀

hm graph uses uploadSessions to represent ongoing uploads ... and the properties expirationDateTime, nextExpectedRanges and uploadUrl are mostly covered by the current interface. ocis / reva actually knows about multiple protocols ... but that is nothing to expose via tha graph api ...

hm

@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

@aduffeck I completely changed the upload interface. I adopted UploadSessions to differentiate it from tus uploads. similar to tus.Info every UploedSession has a Purge() that can be used to completely remove the upload. I don't fully know if we need to emit an event, I think so, but I need to check in which cases.

The ListUploadSessions takes a filter that allows filtering the uploads by expired status, isprocessing flag or id. This allows all current usages and even more granular listing of uploads by the cli.

I also added the antivirus state, which is set during postprocessing. This can be used to list all not yet scanned uploads ...

your opinion?

@butonic butonic force-pushed the exctract-uploadmetadata-interface branch 2 times, most recently from 0ceef88 to 69ec64f Compare November 30, 2023 13:56
@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

tracking the tus upload-expires bug in #4379

@butonic butonic force-pushed the exctract-uploadmetadata-interface branch from 670b4c2 to e8b0c90 Compare November 30, 2023 15:11
@butonic butonic marked this pull request as ready for review November 30, 2023 15:17
@butonic butonic requested review from a team, labkode and glpatcern as code owners November 30, 2023 15:17
@aduffeck
Copy link
Contributor

@aduffeck I completely changed the upload interface. ...
your opinion?

lgtm, as discussed in zoom 👍

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic force-pushed the exctract-uploadmetadata-interface branch from 5837476 to f37fe42 Compare November 30, 2023 15:30
@butonic butonic changed the title extract upload manager interface extract UploadSessionLister interface Nov 30, 2023
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Generally looks good but the linter is not happy

Additional Question: Did you run a full-ci pipeline on ocis to make sure these changes don't break ocis pipeline?

pkg/storage/uploads.go Show resolved Hide resolved
pkg/storage/uploads.go Outdated Show resolved Hide resolved
pkg/storage/uploads.go Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload.go Outdated Show resolved Hide resolved
pkg/storage/utils/decomposedfs/upload/processing.go Outdated Show resolved Hide resolved
pkg/storage/uploads.go Show resolved Hide resolved
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic
Copy link
Contributor Author

butonic commented Nov 30, 2023

@kobergj I hope I made the linter happy ;-) will create an ocis bump right away

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@butonic butonic merged commit a389ddc into cs3org:edge Dec 1, 2023
9 checks passed
@butonic
Copy link
Contributor Author

butonic commented Dec 1, 2023

ocis pr was green, will rebase #4148 now

@butonic butonic deleted the exctract-uploadmetadata-interface branch December 1, 2023 12:22
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.

3 participants