Skip to content

[CON-118 + CON-126] (Content Node) Paginate getNodeUsers in snapback - #3064

Merged
theoilie merged 4 commits into
masterfrom
theo-paginate-getNodeUsers
May 11, 2022
Merged

[CON-118 + CON-126] (Content Node) Paginate getNodeUsers in snapback#3064
theoilie merged 4 commits into
masterfrom
theo-paginate-getNodeUsers

Conversation

@theoilie

@theoilietheoilie commented May 10, 2022

Copy link
Copy Markdown
Contributor

Description

  • Update Content Node to consume the pagination from [CON-118 + CON-126] (Discovery Node) Paginate /v1/full/users/content_node route #3071. This allows the first step of the state machine (getNodeUsers()) to quickly fetch a slice of users instead of having to fetch all users and then manually slice them.
  • Expose new config option snapbackUsersPerJob via health check
  • Maintain backwards compatibility by performing the manual slicing if getNodeUsers() returns the full list of users instead of a paginated slice

Tests

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

  • Look through Content Node logs containing processStateMachineOperation or StateMachineQueue and verify that lastProcessedUserId is increasing by snapbackUsersPerJob (from the health check) each time

@vicky-gvicky-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good stuff! i do have some comments / concerns. let's discuss this via voice chat

Comment threadcreator-node/src/app.js Outdated
Comment threadcreator-node/src/app.js Outdated
Comment threadcreator-node/src/config.js
Comment threadcreator-node/src/index.ts Outdated
Comment threadcreator-node/src/snapbackSM/peerSetManager.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js Outdated
Comment threadcreator-node/src/snapbackSM/snapbackSM.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js Outdated
Comment threaddiscovery-provider/src/api/v1/users.py Outdated
Comment threaddiscovery-provider/src/api/v1/users.py Outdated
@theoilie

Copy link
Copy Markdown
ContributorAuthor

thanks @vicky-g! I resolved all your comments related to bull-board here and addressed them in another PR (#3055). also going to break apart the Discovery Node portion into its own PR so no need to re-review til those PRs get through and I rebase this one off of them 😄

@theoilie
theoilieforce-pushed the theo-paginate-getNodeUsers branch from 187b66b to d000fbeCompareMay 11, 2022 17:29
@theoilietheoilie changed the title Paginate getNodeUsers (v1/full/users/content_node/all)[CON-118] (Content Node) Paginate getNodeUsers in snapbackMay 11, 2022
@theoilie
theoilie requested a review from vicky-gMay 11, 2022 18:52
Comment threadcreator-node/src/snapbackSM/peerSetManager.js
Comment threadcreator-node/src/snapbackSM/peerSetManager.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js
@theoilie
theoilie merged commit 861d46f into masterMay 11, 2022
@theoilie
theoilie deleted the theo-paginate-getNodeUsers branch May 11, 2022 22:13

@SidSethiSidSethi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sweet! generally looks great, couple small comments
also if you could update pr desc with:

  • remove part about adding DN pagination since thats in separate PR, instead add reference to that PR
  • explicitly call out how you handled back-compat
  • explain why this pagination was added
  • in Test part, explicitly call out testing of both new and old flows, ideally with some assurance of both pagination & modulo based increments working correctly

Comment threadcreator-node/src/utils.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js
Comment threadcreator-node/src/snapbackSM/snapbackSM.js
@theoilie

Copy link
Copy Markdown
ContributorAuthor

updated PR description with everything requested and made a followup PR (#3102) for the rest of your comments @SidSethi thanks!

@SidSethiSidSethi changed the title [CON-118] (Content Node) Paginate getNodeUsers in snapback[CON-118 + CON-126] (Content Node) Paginate getNodeUsers in snapbackMay 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
@AudiusProjectAudiusProject deleted a comment from linearBotSep 11, 2023
@AudiusProjectAudiusProject deleted a comment from linearBotSep 11, 2023
Sign up for freeto 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

@theoilie@SidSethi@vicky-g