Conversation
7d77c41 to
2ed7659
Compare
| @@ -0,0 +1,166 @@ | |||
| """Temporary private streaming adapters for object-storage response bodies. | |||
There was a problem hiding this comment.
Ideally none of these wrappers are necessary, and django-storages would allow us to do something like this natively: jschneier/django-storages#1568
But for now, we cannot, and the buffering implementation is genuinely unsuitable for sufficiently large files (context: a user has reported that they are storing files ranging up to 60 or even 300 gigabytes)
There was a problem hiding this comment.
Obviously this is relatively invasive, so we need to be very careful with testing.
There was a problem hiding this comment.
Alternatively, we could temporarily fork django-storages and add better APIs directly to the implementation. It is not in a great place maintenance-wise anyway currently.
There was a problem hiding this comment.
Here is what the upstream PR looks like jschneier/django-storages#1569
There was a problem hiding this comment.
Copied over to our new fork pulp/django-storages-internal-use-only#1
|
@mdellweg I'm marking this draft because it probably needs more extensive testing, however, I would still like feedback on the direction and approach. e.g. if you want to try the "vendor/fork django-storages" approach. Both the issue itself and the upstream status of the library is a real issue we're going to need to deal with one way or the other. |
|
First, I don't know what the best approach is. Each approach has pro's and con's. |
cc3d9d2 to
2b3471d
Compare
|
@mdellweg I updated the PR to use a forked copy, which for now just points to my own branch. The tests need work (not ready for a full review yet) but the implementation is much cleaner. WDYT? |
This seems to follow separation of concerns, and I approve of that. Keeping the storages code separated means we can still explore all options like forking/vendoring. |
2b3471d to
a95c585
Compare
Bypass django-storages eager file buffering when the content app proxies S3 or Azure artifacts. Preserve the generic storage fallback and HTTP range behavior. closes pulp#7806 Assisted By: Codex (GPT-5) Terra 5.6
a95c585 to
632e6c2
Compare
|
|
||
| domain = domain_factory(storage_class=storage_class, redirect_to_object_storage=False) | ||
| artifact = random_artifact_factory(pulp_domain=domain.name, size=32) | ||
| content = gen_object_with_cleanup( |
There was a problem hiding this comment.
delete on content does not seem right.
Also would it not be easier to just upload a few random bytes into the file repository?
| asyncio.run(_test_response_closes_object_stream_after_write_failure(monkeypatch)) | ||
|
|
||
|
|
||
| async def _test_response_closes_object_stream_after_write_failure(monkeypatch): |
There was a problem hiding this comment.
I'm certain, we allow async tests here. No need for the extra wrapper.
This branch has not been deployed
2 participants
Bypass django-storages eager file buffering when the content app proxies S3 or Azure artifacts. Preserve the generic storage fallback and HTTP range behavior.
closes #7806
Assisted By: Codex (GPT-5) Terra 5.6
📜 Checklist
See: Pull Request Walkthrough