[NFT Metadata Crawler] Asset Uploader API, Worker, Throttler #14837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 1h 30m total CI duration on this PR
|
just-in-chang
force-pushed
the
justin/asset-uploader-worker
branch
from
b06b02d to
44bfd22
Compare
jillxuu
reviewed
Oct 3, 2024
| ) -> impl IntoResponse { | ||
| match context.upload_asset(&request.url).await { | ||
| Ok(res) => { | ||
| let res = res.into_response(); // TODO: How to log response body? |
There was a problem hiding this comment.
wait which service is updating the db for request status and response? is it throttler or worker?
btw do we need to log the response if postgresdb will maintain request status and response?
There was a problem hiding this comment.
It's the throttler that performs the db updates, and it's just nice to log the body to help with debugging
| async fn upload_asset(&self, url: &Url) -> anyhow::Result<impl IntoResponse> { | ||
| let hashed_url = sha256::digest(url.to_string()); | ||
| let client = Client::builder() | ||
| .timeout(Duration::from_secs(MAX_ASSET_UPLOAD_RETRY_SECONDS)) |
There was a problem hiding this comment.
should this live in the config instead of const var? we might not know right away what's the best seconds to retry for
There was a problem hiding this comment.
This is just an arbitrarily high number. Theoretically, we should be nowhere near this limit because of the size limit for cloudflare.
ecosystem/nft-metadata-crawler/src/asset_uploader/worker/mod.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+101
to
+233
| /// Converts a reqwest response to an axum response | ||
| /// Only copies the response status, response body, and Content-Type header |
There was a problem hiding this comment.
wondering is it worth having these two libraries for incoming and outgoing http requests? what if we just a single rust library for requests? chatgpt suggested hyper?
There was a problem hiding this comment.
we previously used reqwest in nft netadata crawler, which is why i used reqwest here. Also hyper makes it very hard to submit multipart requests bc hyper too low level and doesnt support it out of the box
jillxuu
approved these changes
Oct 8, 2024
larry-aptos
approved these changes
Oct 22, 2024
just-in-chang
changed the title
just-in-chang
changed the title
just-in-chang
force-pushed
the
justin/asset-uploader-worker
branch
from
6cc119e to
7ec2b26
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adaptation of the old Asset Uploader to the Asset Uploader Worker from the new design.
Design: https://www.notion.so/aptoslabs/WIP-Asset-Uploader-cb33138051324d1fa03115ef1b55ed0b
How Has This Been Tested?
Key Areas to Review
The service is even simpler than before. Please just look at overall logic and critique my coding style for Rust best practices.
Type of Change
Which Components or Systems Does This Change Impact?
Checklist