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

[CON-517] Add a route to translate trackBlockchainId to copy320 CID #4377

Merged
merged 7 commits into from
Nov 22, 2022

Conversation

jonaylor89
Copy link
Contributor

@jonaylor89 jonaylor89 commented Nov 17, 2022

Description

This PR adds a new route /batch_id_to_cid that translates a batch of trackBlockchainIds to copy320 CIDs.

The return schema looks like

{
    'trackBlockchainId': 'multihash'
}

Tests

Tested locally by creating a user, uploading a track, and running curl to fetch the copy320.

And he's a test on the node cli to remove any suspicion that the functional code used to create the actually does work.
Screenshot 2022-11-18 at 16 45 39

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

@jonaylor89 jonaylor89 changed the title add new batch route [CON-517] Add a route to translate trackBlockchainId to copy320 CID Nov 17, 2022
@jonaylor89 jonaylor89 marked this pull request as ready for review November 17, 2022 22:07
creator-node/src/routes/files.js Outdated Show resolved Hide resolved
}

// make query
const queryResults = await models.File.findAll({
Copy link
Contributor

Choose a reason for hiding this comment

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

have you benchmarked this query on prod DB? should make sure it uses an index / is performant
(can confirm this with explain analyze <query>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't yet. I'll check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2022-11-18 at 17 23 06

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it does use an index

Copy link
Contributor

Choose a reason for hiding this comment

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

sweet - i wanna see what this looks like against prod DB with ~50k trackIDs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Voila

Copy link
Contributor

Choose a reason for hiding this comment

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

perf

Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

query looks good - lets make sure it is sufficiently performant and that the batch size is reasonable

@jonaylor89 jonaylor89 merged commit 8cc7a18 into main Nov 22, 2022
@jonaylor89 jonaylor89 deleted the jn-con-517 branch November 22, 2022 19:35
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants