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

feat!: return shards as array from upload/add #56

Merged
merged 7 commits into from
Nov 30, 2022
Merged

Conversation

olizilla
Copy link
Contributor

@olizilla olizilla commented Nov 29, 2022

Change upload/add to return items with a shards array instead of expanding them out to many root#shard rows.

Stores shards as a String Set in dynamo. Multiple invocations of upload/add with the same root store the union of all the shards from each invocation.

Fixes up type definitions and some tidying up from #48

Removes issuer and invocation from upload/add response, we're not using them, and there is an open question of how to store them...

Question

  • Do we want to store CIDs for all the invocations that change a given upload item in the same document? or seperate doc in the same table, or just not? We store a single invocation CID in the store table as we dont have any updates there, so this question hasn't come up yet. The answer connects to how we imaging the event/ucan log table working too see Integrate UCAN_LOG_TABLE #18

Fixes #52

License: MIT
Signed-off-by: Oli Evans oli@protocol.ai

Change `upload/add` to return items with a `shards` array instead of expanding them out to many root#shard rows.

Stores shards as a String Set in dynamo. Multiple invocations of `upload/add` with the same root store the union of all the shards from each invocation.

Fixes #52

License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr56 November 29, 2022 22:12 Inactive
License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy
Copy link

seed-deploy bot commented Nov 29, 2022

View stack outputs

@seed-deploy seed-deploy bot temporarily deployed to pr56 November 29, 2022 22:20 Inactive
@seed-deploy seed-deploy bot temporarily deployed to pr56 November 29, 2022 22:24 Inactive
@seed-deploy
Copy link

seed-deploy bot commented Nov 29, 2022

Stack outputs updated

License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr56 November 29, 2022 22:40 Inactive
Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

This is looking good! I am super happy with new Upload structure 🤩

Tests are failing though

api/tables/index.js Outdated Show resolved Hide resolved
}

export interface UploadTable {
exists: (space: DID, root: AnyLink) => Promise<boolean>
insert: (item: UploadItemInput) => Promise<UploadItemOutput[]>
insert: (item: UploadAddInput) => Promise<UploadAddResult>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we follow store pattern and use Output for return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we've not got a consistent pattern yet. The store table abstraction doesn't have enough info to create the StoreAddResult as the service includes other field like status and the signed url... so we go

StoreAddInput -> StoreAddOutput -> StoreAddResult

whereas in for upload/add we already have all the info for the service level result. I'll split those types out to match if we need it.

api/test/service/upload.test.js Outdated Show resolved Hide resolved
api/test/service/upload.test.js Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Contributor

Do we want to store CIDs for all the invocations that change a given upload item in the same document? or seperate doc in the same table, or just not? We store a single invocation CID in the store table as we dont have any updates there, so this question hasn't come up yet.

I think maybe we don't need them if we are also storing the UCAN Log Table. But I might be missing out on something. Let's create an issue to get a broader group discussion on that without blocking this PR

@olizilla
Copy link
Contributor Author

oh my. the test is failing because it takes more than a second for the function under test to write to the db and return the value, so where i give 1000ms grace for an insertedAt value is not long enough!

License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr56 November 30, 2022 13:07 Inactive
License: MIT
Signed-off-by: Oli Evans <oli@protocol.ai>
@seed-deploy seed-deploy bot temporarily deployed to pr56 November 30, 2022 13:17 Inactive
Co-authored-by: Vasco Santos <santos.vasco10@gmail.com>
@seed-deploy seed-deploy bot temporarily deployed to pr56 November 30, 2022 13:20 Inactive
@olizilla olizilla merged commit 236c981 into main Nov 30, 2022
@olizilla olizilla deleted the upload-shards-as-set branch November 30, 2022 13:23
alanshaw pushed a commit to storacha/w3up that referenced this pull request Dec 5, 2022
- update tests and types to handle upload/list items with a shards array
property.
- update upload/add fn to return an UploadAddResponse as shards out may
be more than shards in.
- update README

see also: storacha/w3infra#56

Signed-off-by: Oli Evans <oli@protocol.ai>
gobengo pushed a commit to storacha/w3up that referenced this pull request Apr 11, 2023
- update tests and types to handle upload/list items with a shards array
property.
- update upload/add fn to return an UploadAddResponse as shards out may
be more than shards in.
- update README

see also: storacha/w3infra#56

Signed-off-by: Oli Evans <oli@protocol.ai>
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.

Store upload/add shards as a Set.
2 participants