-
Notifications
You must be signed in to change notification settings - Fork 106
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
[AUD-391] In content-node track stream, add a fallback to enable streaming if track association didn't complete #1284
Conversation
} | ||
|
||
const { multihash } = await models.File.findOne({ | ||
let fileRecord = await models.File.findOne({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more history here btw: fd29e3b#diff-cea0356d19f998f416dcaca2141fb24156c529095b61daebb71f8e3316071f32R524
creator-node/src/routes/tracks.js
Outdated
} | ||
|
||
// make sure all track segments have the same sourceFile | ||
const segments = metadataJSON.track_segments.map(segment => segment.multihash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why all these sanity checks? it would seem maybe better to err on the side of it being ok if the segments don't line up? / does this really happen often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should almost never happen but this is useful bc if we pull the sourcefile off any random multihash file entry, it could map to multiple tracks (only if all uploaded by same cnodeUSerUUID) and we could end up streaming the wrong track. given that this is infrequent it seems valuable to do? like all of this should be < 1 sec only the first time request before its cached
but also can see coutnerarg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only case when this should happen is if the user uploads the same song multiple times and it remains unassociated after multiple failed attempts. we can just treat this as warnings and log out and continue, but the tradeoff is if there was a remix etc, there's an edge case where we could play the wrong song if a CID overlaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to release, aside from ray's comment about only hitting redis if not already found from db
some comments but not a blocker for hotfix release, as long as we address before master merge
creator-node/src/routes/tracks.js
Outdated
} | ||
|
||
// make sure all track segments have the same sourceFile | ||
const segments = metadataJSON.track_segments.map(segment => segment.multihash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should almost never happen but this is useful bc if we pull the sourcefile off any random multihash file entry, it could map to multiple tracks (only if all uploaded by same cnodeUSerUUID) and we could end up streaming the wrong track. given that this is infrequent it seems valuable to do? like all of this should be < 1 sec only the first time request before its cached
but also can see coutnerarg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for fixing the redis check. again, good to go for for hotfix, can look over again pre master merge, can prob clean some stuff up before
@raymondjacobson @SidSethi can. you look again? just addressed both your comments. also i realized i didn't branch off the current cn hotfix, so let me make that branch too. this we can use to merge onto master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incredible, lets 🚢
Description
https://www.notion.so/audiusproject/Content-Node-Stream-Route-Bugfix-b585d68f89b24161a776bc70447eae7c
For a small subset of users, the track association doesn't complete. That means they don't have a record in the Tracks table. However, that doesn't mean that we can't gather enough information to stream the track. This removes the check in the Tracks table since that could fail in a small number of cases and adds fallback checks to discovery to get the metadata multihash and then attempts to lookup the copy320 file with a matching sourceFile from the trackSegments. There's also a redis cache to prevent an excessive number of discovery calls
Tests
Manually set the fileRecord to null and check that it fetches the data and does the logic successfully from discovery. Subsequent loads should use the redis cache. Attached are logs checking the regular flow and the cached flow. Look for the arrows, which are relevant comments