Re-work zarr multipart upload - #2869
Conversation
Just to make sure -- all prior uploaded zarrs will stay single-part, and only new ones, and new uploaded with the newer version of dandi-cli will be "multipart"? |
Correct. |
ff1e91d to
9d61609
Compare
|
|
||
| @classmethod | ||
| def initialize_zarr_multipart_upload(cls, etag, size, zarr: ZarrArchive, chunk_key: str): | ||
| object_key = zarr.s3_path(chunk_key.lstrip('/')) |
There was a problem hiding this comment.
This changes an invariant we implicitly have that Upload rows are 1:1 with S3 blobs. With this change, I believe you could have a situation where you create multiple Upload objects that point at the same S3 object.
One thing this breaks is Upload garbage collection - see https://github.com/dandi/dandi-archive/blob/master/dandiapi/api/services/garbage_collection/upload.py#L59; just having one stale Upload pointing at an S3 object is enough for it to get deleted out from under others that point at it
| @@ -15,14 +15,15 @@ | |||
| from rest_framework.response import Response | |||
There was a problem hiding this comment.
In general, it seems you're mixing a lot of zarr-only logic into the upload view and model class. Which clashes with the motivation behind the separate zarr and api apps. My main concern is that it's becoming difficult to trace the upload path of AssetBlobs and Zarrs independently.
I'm not sure how to best address this. The first thing that comes to mind is to move the zarr upload logic into the zarr app, and just accept some slight duplication, but I saw you mentioned you already tried that in the older PR. I'm curious what you think about the negatives of that approach vs. this one.
| constraints = [ | ||
| # Ensure that there's exactly one of dandiset or zarr | ||
| models.CheckConstraint( | ||
| name='dandiset-zarr-xor', | ||
| condition=models.Q(dandiset__isnull=True, zarr__isnull=False) | ||
| | models.Q(dandiset__isnull=False, zarr__isnull=True), |
There was a problem hiding this comment.
TODO: Add constraint for unique blobs per dandiset/zarr
There was a problem hiding this comment.
And/or split up the models
This is to preserve existing behavior with how exceptions are handled in the drf_utils module
9d61609 to
3be87c7
Compare
4 participants
Supersedes #2865
This reworks the zarr multipart upload procedure by firmly distinguishing between multi-part zarrs, and single-part zarrs. This is done via a
multipartflag on the zarr model itself. This allows all existing single-part zarrs to remain as such, while enabling multi-part upload capability, without any inherent size threshold.This also unifies the zarr and asset blob multipart upload endpoints into the singular existing asset blob upload endpoint. The original implementation in #2784 has been since reverted (see #2878). It was never deployed to production.
This change is intended to be backwards compatible with the CLI, as it simply extends the fields present in both the upload initialize and upload complete endpoints. The existing CLI PR (dandi/dandi-cli#1839) that was pointed at the previous change in #2784 can either be reworked, or replaced with a new PR.