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-151] Refactor reconciliation queue #3174

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

theoilie
Copy link
Contributor

@theoilie theoilie commented May 28, 2022

Description

  • Add state reconciliation queue skeleton
  • Make state monitoring jobs return data about users instead of syncs and update-replica-set-operations
  • Rename job processors to end with .jobProcessor.js
  • Make minor miscellaneous test changes from previous feedback

The big change with this skeleton is that the monitoring queue outputs the building blocks instead of the actual potential sync ops and update replica set ops. Two other monitoring queue jobs will use those building blocks to separately determine syncs and reconfigs, which will be passed individually to the reconciliation queue. This decoupling also helps make _aggregateOps() easier to read (Linear card for that) and makes sense because determining syncs is much simpler and doesn't need all the complicated logic that determining reconfigs uses.

After this refactor there will be 4 reconciliation jobs (all part of the same queue but different job types, processor functions, and concurrencies):

  1. Issue Sync Request Job: determines if a single user should be synced (based on clock value) and issues a request to the user's nodes to sync
  2. Update Replica Set Job: performs a reconfiguration of a single user's replica set
  3. Recurring Sync Job: triggered by Sync Request Job from another node. Performs the actual sync
  4. Manual Sync Job: same as snapback's manual sync queue but as a job type within the larger Reconciliation Queue

The monitoring queue will wait for State Monitoring Jobs to complete and then use the output of those jobs to enqueue Sync Request Jobs and Update Replica Set Jobs for individual users.

Tests

I ran A up locally to make sure there were no new errors in the logs and verified that bull dashboard shows the rate limited monitoring jobs still running successfully with the updated output. New tests will come after functionality is reshuffled. Existing tests are updated to pass:

  • StateMonitoringQueue.test.js
  • monitorState.jobProcessor.test.js (renamed from processStateMonitoringJob.test.js)

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

There shouldn't be any new errors containing the following:

  • processStateMonitoringJob ERROR
  • StateMonitoringQueue ERROR
  • StateReconciliationQueue ERROR

@theoilie theoilie added the content-node Content Node (previously known as Creator Node) label May 28, 2022
@theoilie theoilie requested review from SidSethi and vicky-g May 28, 2022 02:47
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.

generally skeleton makes sense

we should maybe clarify the naming a bit to distinguish btwn outgoing sync request and the processing of a sync request. not sure of best approach
telling a node to sync against yourself vs another node teling you to sync against it vs a user telling you to sync from another node

not sure of best solution, but the naming currently doesn’t seem to do a great job of conveying the intent - wdyt

i really like the names you used for the queue processor types, very clear

@theoilie
Copy link
Contributor Author

thanks for the review @SidSethi. I was thinking about the sync naming as well and didn't come to a great name. maybe just adding "outgoing" and "local" would help, although it would make the processor name long -- something like:

  • processIssueOutgoingSyncRequestsJob.js to send sync requests to this node or other nodes
  • processExecuteLocalSyncJob.js to execute (on this node) manual and recurring requests that come in from this node or other nodes

@theoilie theoilie marked this pull request as ready for review May 31, 2022 23:12
@theoilie theoilie requested review from SidSethi and removed request for vicky-g May 31, 2022 23:12
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.

my impression was that each stateReconciliation job would handle a single user / operation? eg the final step of each stateMonitoringQueue job would be to enqueue jobs for every single operation by job type into stateREconciliation
it seems like currently each stateREcon job handles multiple ops?

otherwise looks fine but since this is mostly boilerplate/skeleton code, not a whole lot to review - generally all good

maybe easier to sync via slack quickly

@theoilie
Copy link
Contributor Author

theoilie commented Jun 1, 2022

@SidSethi I thought about it a little more after we synced offline, and breaking apart finding syncs vs finding reconfigs into separate monitoring jobs makes sense to me. I'm holding off on putting that into this PR so I can flesh it out locally and make sure it looks reasonable (and just combine into 1 job if not). for now I just updated the recon jobs to only handle one user at a time

@SidSethi
Copy link
Contributor

SidSethi commented Jun 1, 2022

@SidSethi I thought about it a little more after we synced offline, and breaking apart finding syncs vs finding reconfigs into separate monitoring jobs makes sense to me. I'm holding off on putting that into this PR so I can flesh it out locally and make sure it looks reasonable (and just combine into 1 job if not). for now I just updated the recon jobs to only handle one user at a time

sounds good!

@theoilie theoilie requested a review from SidSethi June 1, 2022 17:33
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 good! i don't see this as a particularly valuable change to merge in current state, vs waiting to fill in the skeleton more (even if not wired up) and merge together. your call tho
test coverage is nice

also pls update pr description with latest and greatest :)

@theoilie
Copy link
Contributor Author

theoilie commented Jun 1, 2022

I'd rather get it merged so the next PR won't have as much bloat -- it's definitely going to be a big one. updated the PR description and merging once tests pass

@theoilie theoilie merged commit 0faad13 into master Jun 1, 2022
@theoilie theoilie deleted the theo-refactor-reconciliation-queue branch June 1, 2022 18:35
sliptype pushed a commit that referenced this pull request Sep 10, 2023
sliptype pushed a commit that referenced this pull request Sep 10, 2023
[eb23ea0] [C-2309] Fix track title hover state (#3197) Dylan Jeffers
[b82a6ee] [C-2303] Ensure consistent play/active state for entity tiles (#3195) Dylan Jeffers
[3faedfb] [C-2223] Flex on Finnuh (#3196) Raymond Jacobson
[d1cd884] [C-2271] Update data passed to edit playlist form to update playlist properly (#3183) Kyle Shanks
[7837460] [C-2187] Improve sign-up error messages (#3194) Dylan Jeffers
[e200477] [C-2287] Update wallet connect language, flags, icons (#3192) Dylan Jeffers
[cc0b56d] allow create notification to handle playlist id as array or int (#3190) sabrina-kiam
[d357b3b] [C-2431] Add tastmaker notification to mobile (#3191) Dylan Jeffers
[83a6d36] No playlists text fix C-2296 (#3189) nicoback2
[0e44281] [PLAT-847]: Fix user subscription create album notification copy (#3162) sabrina-kiam
[7da3e10] [C-2412] Opaque id deeplinks (#3188) Sebastian Klingler
[4ff1c53] [C-2418] Fix mobile web open in app banner (#3173) Raymond Jacobson
[22a1240] [C-1504] Prevent user from following themselves (#3187) Dylan Jeffers
[465182a] [C-2306] Fix sign-in sign-up invalid email bug (#3186) Dylan Jeffers
[2cec764] [C-1750] Refactor top tags (#3180) Dylan Jeffers
[7ca864f] Fix Snap sharing with weird track names C-2081 (#3176) nicoback2
[761187a] Fix missing track on Sammie's page C-2342 (#3182) nicoback2
[41c9cf5] Fix track upload cover art name (#3138) Theo Ilie
[cfb0b96] [C-2428] Handle null in cache map handle/permalink adds (#3184) Andrew Mendelsohn
[0559902] [PAY-1127] Improve mobile chats performance: remove index dep (#3170) Reed
[961649a] Use RNGH touchables for mobile chats (#3174) Reed
[be5d2a5] [C-2427] Fix preinstall checks for ruby and python versions (#3179) Andrew Mendelsohn
[48f8d4c] Update readme with deps, remove sudo (#3178) Reed
[fe79400] Disallow sharing premium content to social media, remove feature flags C-2334 C-2420 (#3172) nicoback2
[776596f] [PAY-829][PAY-1044] Desktop: Chats search users modal permissions/blocks and profile page permissions/blocks (#3156) Marcus Pasell
[e4df304] Fix scrolling on ChatListScreen (#3171) Reed
[6f35699] [PAY-1041][PAY-1040][PAY-1126] Mobile search users screen improvements (#3159) Reed
[d9e302b] [PAY-1143] Desktop: Overflow menu on Chat Header (#3169) Marcus Pasell
[798a664] Fix edit profile button not working after you submit the first time (#3163) nicoback2
[1d78774] [PLAT-814] Use sdk discovery-node-selection in libs (#3125) Dylan Jeffers
[58005c4] [PAY-799] DMs: Link previews (#3096) Marcus Pasell
[7e3d44c] [PAY-924][Desktop] DMs: Blocking/Unblocking Wire Up/UI (#3155) Marcus Pasell
[7038d81] [C-2347] Improve popover zIndex (#3168) Dylan Jeffers
[3eb62fe] [C-2364] Fix missing artist-card badges on android (#3165) Dylan Jeffers
[64ffb7f] Update @audius/sdk to 2.0.3-beta.4 (#3160) Marcus Pasell
[6e5306c] [QA-443] Fix cache case matching for handles and permalinks (#3167) Andrew Mendelsohn
[84dcb5a] [C-2059] Show toast when email check fails due to blocked user (#3166) Sebastian Klingler
[be855a4] [C-2419] Fix offline lineup loading (#3164) Andrew Mendelsohn
[920e25b] Fix permissions message for adding to photos C-2379 (#3161) nicoback2
[78cbdfa] [PLAT-819]: Fix copy for repost and save of albums notifications (#3151) sabrina-kiam
[10617a2] Mobile chats use push instead of navigate (#3154) Reed
[6131075] [C-2406] Fix repeated edits of same track (#3157) Sebastian Klingler
[3ae27d1] Mobile chats scrolling improvements (#3153) Reed
[e820e3f] [C-2336, C-2388] Playlist style and bug fixes (#3149) Kyle Shanks
[955887f] [C-2406] Null check localStorage (#3150) Sebastian Klingler
[3b31b73] [C-2227] Clear wallet state on sign-out (#3147) Dylan Jeffers
[91940fb] [C-2406] Prevent non finite set of currentTime (#3148) Sebastian Klingler
[d799e30] [C-2404] Fix ios build on xcode 14.3 (#3146) Dylan Jeffers
[186a086] [C-2398] Clean up playlist-update feature flags (#3145) Dylan Jeffers
[edb3142] update to xcode 14.2.0 (#3107) Raymond Jacobson
[4add953] [PLAT-835] Fix tastemaker twitter button (#3144) Dylan Jeffers
[e68ada9] [PLAT-834]: Fix notifications last seen new tag (#3143) sabrina-kiam
@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
content-node Content Node (previously known as Creator Node) size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants