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-118 + CON-126] (Discovery Node) Paginate /v1/full/users/content_node route #3071

Merged
merged 5 commits into from
May 12, 2022

Conversation

theoilie
Copy link
Contributor

@theoilie theoilie commented May 10, 2022

Description

  • Add pagination to Discovery Node for /v1/full/users/content_node route and SQL query
  • Maintain backwards compatibility by ignoring the 2 new params (prev_user_id and max_users) and just returning all users
  • Cache results of the route for 5 minutes

Example: https://discoveryprovider2.staging.audius.co/v1/full/users/content_node/all?creator_node_endpoint=https://creatornode11.staging.audius.co&prev_user_id=0&max_users=100 will only return 100 users, but if the Discovery Node isn't updated to this PR, it will just return all users.

Pagination works by accepting prev_user_id and max_users params. It will return all user IDs (in ascending order of user_id) that are greater than prev_user_id, up to max_users number of users. The consumer of this pagination should then pass the highest user_id as the prev_user_id into the subsequent pagination request.

Tests

Verified query by execing into Postgres container and running it. Also hit the endpoint manually with different params locally -- e.g. http://dn1_web-server_1:5000/v1/full/users/content_node/all?creator_node_endpoint=http://cn1_creator-node_1:4000&prev_user_id=3&max_users=2

How will this change be monitored? Are there sufficient logs?

Search for v1/full/users/content_node/all in Discover Node logs.

@theoilie theoilie requested review from vicky-g and SidSethi May 10, 2022 22:54
Copy link
Contributor

@vicky-g vicky-g left a comment

Choose a reason for hiding this comment

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

:shipit:

discovery-provider/src/queries/get_users_cnode.py Outdated Show resolved Hide resolved
discovery-provider/src/queries/get_users_cnode.py Outdated Show resolved Hide resolved
discovery-provider/src/api/v1/users.py Show resolved Hide resolved
discovery-provider/src/api/v1/users.py Outdated Show resolved Hide resolved
discovery-provider/src/api/v1/users.py Outdated Show resolved Hide resolved
@theoilie theoilie requested a review from SidSethi May 11, 2022 18:35
@theoilie theoilie changed the title (Discovery Node) Paginate /v1/full/users/content_node route [CON-118] (Discovery Node) Paginate /v1/full/users/content_node route May 11, 2022
discovery-provider/src/api/v1/users.py Outdated Show resolved Hide resolved
discovery-provider/src/api/v1/users.py Outdated Show resolved Hide resolved
discovery-provider/src/queries/get_users_cnode.py Outdated Show resolved Hide resolved
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.

looks great!! couple small comments - can merge after

before that though, can you update pr test description to include the back-compat case i.e. that it returns everything when params not provided etc. logs would be great if possible, but not necessary

also if you could reference the new pagination format in PR desc would be great for future reference, and also mention that this handled back-compat and how it handles it

@SidSethi
Copy link
Contributor

also can you re-run maddog after merging latest master to see if it works with the change that draj merged #3072

@theoilie
Copy link
Contributor Author

also can you re-run maddog after merging latest master to see if it works with the change that draj merged #3072

I had to re-run a couple times because it failed for errors seemed unrelated and intermittent (logged them in Linear) but it passed now so I'm merging!

@theoilie theoilie merged commit 63ed4fc into master May 12, 2022
@theoilie theoilie deleted the theo-paginate-dn-getUsersCnode branch May 12, 2022 23:47
@SidSethi SidSethi changed the title [CON-118] (Discovery Node) Paginate /v1/full/users/content_node route [CON-118 + CON-126] (Discovery Node) Paginate /v1/full/users/content_node route May 17, 2022
sliptype pushed a commit that referenced this pull request Sep 10, 2023
[e804aa1] [C-2248, C-2373] Use playlistUpdates, remove legacyNotifications (#3094) Dylan Jeffers
[824933e] [C-2366] Improve web notification selection performance (#3103) Dylan Jeffers
[4b8edef] [PLAT-696] Add trending-playlists/underground notifications (#3089) Dylan Jeffers
[1f9cf3e] [C-2275] Fix android drawer offsets (#3095) Dylan Jeffers
[fc14c82] [PAY-1063][PAY-1085][PAY-1086] Update UI for inaccessible gated tracks from favorites and history pages (#3100) Saliou Diallo
[b0441f5] [C-2365] Update play buttons on web and mobile to show resume when track is current (#3101) Kyle Shanks
[453910f] [C-2378] Add upload v2 feature flag (#3099) Sebastian Klingler
[962a6df] [C-2337] Remove reachability mobile web (#3090) Raymond Jacobson
[4ad5cd2] Fix visible collectibles for upload popup (#3093) Saliou Diallo
[c143078] Fix feature flag bug (#3092) Saliou Diallo
[44435b5] Fix upload prompt modal learn more url (#3091) Saliou Diallo
[c9024ad] Use chat.messagesStatus instead of selector (#3087) Reed
[38d43c4] [C-2369] Fix issue where notification poll can break app on signout (#3088) Dylan Jeffers
[90122d9] [PAY-923] DMs: Add desktop entrypoints (#3083) Marcus Pasell
[00f27e8] [PAY-907] Mobile chat reactions (#3020) Reed
[4678b89] DMs: Fix broken typecheck on main (#3086) Marcus Pasell
[756ade4] [PAY-1000][PAY-1084][PAY-1096][PAY-1097][PAY-1098] - More gated content fixes (#3085) Saliou Diallo
[820aa9d] Fix upload and repost probers tests and lint (#3076) Sebastian Klingler
[345607e] [C-2320] Fix profile socials alignment (#3079) Dylan Jeffers
[569199c] Fix prod build timeout (#3084) Sebastian Klingler
[12f6c22] Remove ports for local dev (#3082) Theo Ilie
[1940618] Fix broken Main build due to typeerror (#3080) Marcus Pasell
[eb8d47e] [PAY-1082] DMs: Dedupe sent messages (#3066) Marcus Pasell
[50a11c3] Update SDK to 2.0.3-beta.0 (#3078) Marcus Pasell
[c420fbb] Clean up NPM package lock (#3077) Marcus Pasell
[35d1124] [C-2327] Add playlist updates slice (#3063) Dylan Jeffers
[59862ad] [C-2344] Update the web playbar scrubber to respect the playback speed of podcasts (#3075) Kyle Shanks
[ffeb0d3] [C-2349] Default download on wifi only to false (#3074) Andrew Mendelsohn
[cafae41] [C-2325] Fix playlist table date-added column (#3073) Dylan Jeffers
[384a510] [PAY-927] DMs: Empty messages state (#3068) Marcus Pasell
[1132f83] Update @jup-ag/core to 2.0.0-beta.9 (#3072) Marcus Pasell
[49c0ebf] [PAY-1072] Change "Download App" icon on Settings Page (#3067) Marcus Pasell
[928dcaf] [PAY-1056] - More gated content updates and fixes (#3070) Saliou Diallo
[1e1f769] [C-2345] Move PlaybackRate drawer to common drawers map (#3071) Kyle Shanks
[f5d1251] Fix web-dist CI steps (#3069) Sebastian Klingler
[5f89800] Fix heavy rotation playlist on client (#3056) sabrina-kiam
[c0191e2] [C-2316] Add remote config for all oauth verification (#3052) Raymond Jacobson
[40f5627] [PAY-1074][PAY-1075][PAY-1076][PAY-1080] - Update availability settings states + more QA fixes (#3059) Saliou Diallo
[5be60ac] [C-2339] Update podcast control updates to also work for audiobooks (#3065) Kyle Shanks
[163ebf5] [C-2297] Add fallback flag to podcast feature (#3064) Sebastian Klingler
[f206391] [PAY-904] - Add gated content upload prompt (#3057) Saliou Diallo
[1afc4e5] [C-1344] Move probers to monorepo and make tests pass (#3061) Sebastian Klingler
[e198279] Remove random line (#3062) Saliou Diallo
[24a001b] Add playback position logic for mobile (#3051) Kyle Shanks
[d210124] [PAY-1070] Update TabSlider/SegmentedControl slider size on resize (#3044) Marcus Pasell
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
@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.

3 participants