Skip to content

Conversation

@vicky-g
Copy link
Contributor

@vicky-g vicky-g commented Oct 28, 2021

Description

Originally, we waited on ipfs.add to complete for metadata content as discovery nodes directly fetch md content. however, this isn't necessary as at least there is a content node gateway fetch for content. plus our ipfs pods are not stable under load, and this flow is is very critical.

This PR is to remove this await

Tests

How will this change be monitored?

Copy link
Member

@raymondjacobson raymondjacobson left a comment

Choose a reason for hiding this comment

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

This LGTM, but probably worth @dmanjunath or @SidSethi approving as well. Not sure of broader implications in the discovery node retrieval step

Copy link
Contributor

@dmanjunath dmanjunath 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 reasonable until we have v2 out

@vicky-g vicky-g merged commit a09f516 into master Oct 28, 2021
@vicky-g vicky-g deleted the vg-remove-await-md-ipfs-add-2 branch October 28, 2021 19:31
@SidSethi
Copy link
Contributor

looks good to me thanks for getting this patch out - flagging that PR title is a bit misleading. the change you made is not to not await the ipfs add but to not add to daemon at all. change makes sense though!

@vicky-g
Copy link
Contributor Author

vicky-g commented Oct 28, 2021

@SidSethi this PR actually does add content to ipfs daemon! we just don't await it as this with the current ipfs add logic

@SidSethi
Copy link
Contributor

@SidSethi this PR actually does add content to ipfs daemon! we just don't await it as this with the current ipfs add logic

ah i see, the naming of the argument is just misleading addToIPFSDaemon. all this code has already been updated in your other PR so all good

dmanjunath added a commit that referenced this pull request Nov 4, 2021
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.

5 participants