chore: unify /upload and /car uploads #480

olizilla · 2021-09-17T10:20:03Z

Have the API pack uploaded files into a CAR, to make the two paths more similar and ideally simplify the upload to S3 path.
- We need to test this out on staging to see how it impacts the total file size folks can upload, as now we have to have the upload and the CAR in memory at the same time.
Simplify dagSize calcualtion. If it's a dag-pb root, then just derive the cumulative size of the dag in the same way that ipfs does from the sum of the link sizes and the node size. if it's not dag-pb, just use the sum of the block sizes in the CAR.
- This means we dont have to walk the entire DAG and we can get a dagSize figure for dags that are split across mutiple CARs (assuming they stick to the treewalk strategy from carbites, and each contain the root block).

License: (Apache-2.0 AND MIT)
Signed-off-by: Oli Evans [email protected]

Have the API pack uploaded files into a CAR, to make the two paths more similar and ideally simplify the upload to S3 path. There were two ideas here 1. If uploaded files are packed into a CAR by the API before sending to cluster we then only have to deal wth sending CARs to S3, which would be nicer than having to handle both raw files and CARs. 2. We have a lot of different notions of "how big is this thing"... the size of the uploaded CAR, the cumulative size of uploaded files, the sum of the size of each block in gthe CAR, the size or bytes value returned by cluter, and the size of all the blocks in the dag where we understand the ipld codecs and can follow the links, which could be different to the sum of the size of blocks in a CAR if it has redundent or duplicate blocks. It would be good to simplify that. The dream was - To lean on cluster to get the unixFS CumulativeSize when uploading a CAR with files with a unixFS root - ...and pack raw files uploads into a CAR. The reality is - We can't get the unixFS CumulativeSize out of cluster. It might return the FileSize for a CAR with a single file in, but that will be 0 for Directories. - We can't pack files into a unixFS flavour CAR with ipfs-car in a cloudflare worker today as it fails with a about `importScrtips` not being available in CloudFlare workers. License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

olizilla · 2021-09-17T10:48:34Z

Error: Failed to execute 'importScripts' on 'WorkerGlobalScope': the method is not implemented.

Looks like it might be our webpack config trying to split the service worker bundle into chunks, but as it's for upload to the cloudflare that should just be disabled. Am investigating.

this fixes the `importScripts` error License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

olizilla · 2021-09-17T11:12:44Z

Nice! Forcing webpack to not split the bundle fixes the importScripts error.

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

olizilla · 2021-09-20T11:36:14Z

Of note this pushes the api bundlesize up from 686.35KB to 799.28KB

> @web3-storage/[email protected] test:size
> bundlesize

 PASS  ./dist/main.js: 799.28KB < maxSize 1MB (no compression)

vasco-santos · 2021-09-20T12:02:53Z

packages/api/src/upload.js

  }
-
-  return new JSONResponse({ cid })
+  // TODO: do we want to wrap file uploads like we do car uploads from the client?


when receiving a single file, it won't have a path, while directories might have in the name. For directories with several files with no path like name it would likely make sense. I suggest that we should discuss this externally within an issue and keep the current behaviour in this PR context.

packages/api/src/car.js

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

packages/api/src/car.js

vasco-santos · 2021-09-21T11:59:34Z

packages/api/src/car.js

+  // We can't use the `bytes` from cluster where it's a unixfs root partial, as it'll be a shard of the total dag size.
+  // We can't use the `size` from cluster as it's the unixfs FileSize which is 0 for directories, and is not set for raw encoded files, only dag-pb ones.


Maybe we can add this knowledge to the function where we compute the dag size. What do you think?
Reading through the code, it seems strange this placement as it is not clear what it relates to

hmm, fair point, but the bytes and size are properties of the cluster.add response. I'll shuffling it around and see if I can make it make more sense.

packages/api/src/car.js

Co-authored-by: Vasco Santos <[email protected]>

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

vasco-santos

LGTM

alanshaw

Not finished review, submitting what I have...

packages/api/src/car.js

packages/api/src/upload.js

Co-authored-by: Alan Shaw <[email protected]>

olizilla · 2021-09-28T14:53:00Z

packages/api/src/car.js

+  // NOTE: Tsize is optional, but all ipfs implementations we know of set it.
+  // It's metadata, that could be missing or deliberately set to an incorrect value.
+  // This logic is the same as used by go/js-ipfs to display the cumulative size of a dag-pb dag.
+  return pbNodeBytes.length + pbNode.Links.reduce((acc, curr) => acc + (curr.Tsize || 0), 0)


@vasco-santos uh oh. i think this should be:

- return pbNodeBytes.length + pbNode.Links.reduce((acc, curr) => acc + (curr.Tsize || 0), 0) + return pbNodeBytes.byteLength + pbNode.Links.reduce((acc, curr) => acc + (curr.Tsize || 0), 0)

Any suggestions to reliably test this?

fix: dont chunk the api bundle

1d8334e

this fixes the `importScripts` error License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

olizilla added 2 commits September 20, 2021 11:25

chore: derive cumlative size for dag-pb roots

7d266a0

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

chore: fix tests, dont wrap uploads with a dir

7dfcfcd

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

vasco-santos reviewed Sep 20, 2021

View reviewed changes

vasco-santos mentioned this pull request Sep 20, 2021

feat: storage backup #417

Merged

olizilla added 2 commits September 21, 2021 12:09

chore: fix cumulativeSize calculation

6ab9d8a

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

chore: fix lint

9f37a8a

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

olizilla marked this pull request as ready for review September 21, 2021 11:23

olizilla changed the title ~~wip: unify /upload and /car uploads~~ chore: unify /upload and /car uploads Sep 21, 2021

olizilla requested a review from alanshaw September 21, 2021 11:30

vasco-santos requested changes Sep 21, 2021

View reviewed changes

vasco-santos reviewed Sep 21, 2021

View reviewed changes

packages/api/src/car.js Show resolved Hide resolved

olizilla and others added 2 commits September 21, 2021 15:15

Apply suggestions from code review

19b18ae

Co-authored-by: Vasco Santos <[email protected]>

docs: why we can't use the size from cluster

4799255

License: (Apache-2.0 AND MIT) Signed-off-by: Oli Evans <[email protected]>

vasco-santos approved these changes Sep 21, 2021

View reviewed changes

alanshaw reviewed Sep 22, 2021

View reviewed changes

packages/api/src/car.js Outdated Show resolved Hide resolved

packages/api/src/car.js Show resolved Hide resolved

packages/api/src/car.js Outdated Show resolved Hide resolved

alanshaw approved these changes Sep 23, 2021

View reviewed changes

packages/api/src/upload.js Outdated Show resolved Hide resolved

vasco-santos and others added 2 commits September 23, 2021 12:02

chore: apply suggestions from code review

04b98b1

Co-authored-by: Alan Shaw <[email protected]>

chore: fix lint

6d0670b

vasco-santos merged commit b62c8b9 into main Sep 23, 2021

vasco-santos deleted the file-upload-to-car branch September 23, 2021 12:45

vasco-santos mentioned this pull request Sep 23, 2021

fix: upload type in upload should be blob or directory #490

Merged

olizilla mentioned this pull request Sep 23, 2021

chore: bring upload car settings inline with client #491

Merged

vasco-santos mentioned this pull request Sep 27, 2021

Checkbox error handling #482

Closed

olizilla commented Sep 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: unify /upload and /car uploads #480

chore: unify /upload and /car uploads #480

olizilla commented Sep 17, 2021 •

edited

Loading

olizilla commented Sep 17, 2021 •

edited

Loading

olizilla commented Sep 17, 2021

olizilla commented Sep 20, 2021

vasco-santos Sep 20, 2021

vasco-santos Sep 21, 2021

olizilla Sep 21, 2021

vasco-santos left a comment

alanshaw left a comment

olizilla Sep 28, 2021

vasco-santos Sep 28, 2021

		// We can't use the `bytes` from cluster where it's a unixfs root partial, as it'll be a shard of the total dag size.
		// We can't use the `size` from cluster as it's the unixfs FileSize which is 0 for directories, and is not set for raw encoded files, only dag-pb ones.

chore: unify /upload and /car uploads #480

chore: unify /upload and /car uploads #480

Conversation

olizilla commented Sep 17, 2021 • edited Loading

olizilla commented Sep 17, 2021 • edited Loading

olizilla commented Sep 17, 2021

olizilla commented Sep 20, 2021

vasco-santos Sep 20, 2021

Choose a reason for hiding this comment

vasco-santos Sep 21, 2021

Choose a reason for hiding this comment

olizilla Sep 21, 2021

Choose a reason for hiding this comment

vasco-santos left a comment

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

olizilla Sep 28, 2021

Choose a reason for hiding this comment

vasco-santos Sep 28, 2021

Choose a reason for hiding this comment

olizilla commented Sep 17, 2021 •

edited

Loading

olizilla commented Sep 17, 2021 •

edited

Loading