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

[16.0] [mig] attachment_s3 #394

Closed
wants to merge 2 commits into from
Closed

[16.0] [mig] attachment_s3 #394

wants to merge 2 commits into from

Conversation

mileo
Copy link

@mileo mileo commented Nov 15, 2022

No description provided.

@adrienpeiffer
Copy link

I have encountered some problems with the use of the document module. I'll give you some more information soon. @vrenaville Do you plan to work on the subject?

@vrenaville
Copy link
Member

@adrienpeiffer
We don't use S3 anymore, I'm focused on azure, I will try in next week to improve to use Stream more efficiently to load file by chunk , currently it load the file in memory in one big chunk, it's not efficient

@jjscarafia
Copy link

jjscarafia commented Dec 27, 2022

Hi @adrienpeiffer
We've work on a similar migration but using a different approach on PR #401. We've some uses cases where the inheritance of _record_to_stream was not enought and the super call was triying to stream content of an s3 record (For eg. a binary field on website model or something like that).

So me work on an inheritance on an upper method on base_attachment_object_storage.
Is working properly till now.

The load of the file is not optimal and your idea of improvement would be needed

cc @filoquin

@vrenaville
Copy link
Member

@jjscarafia
I think @adrienpeiffer have adopt the same method that I have done for Azure storage.
My method try to avoid monkey patching, the more I can.

@gurneyalex did you have a comment on this ?

@gurneyalex
Copy link
Member

@vrenaville what is annoying is that in both approaches we have to read the attachment from memory and we are not really able to stream it. The approach in #401 is nicer in that if fixes the issue in the base module and I would go for that solution, until we have a way to stream things from the cloud provider.

@jjscarafia
Copy link

I completely agree that we still need to improve and stream it.
I didn't find time yet to show a use case where this face the issue descrived here. Doing as https://github.com/camptocamp/odoo-cloud-platform/pull/401/files works till now on every use case

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale label Jul 23, 2023
@github-actions github-actions bot closed this Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants