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-225] Remove manual sync from snapbackSM #3390

Merged
merged 29 commits into from
Jul 11, 2022
Merged

Conversation

jonaylor89
Copy link
Contributor

@jonaylor89 jonaylor89 commented Jul 6, 2022

Description

This PR moves the manualSyncQueue out of snapbackSM and into the new refactored StateMachineManager.

  1. make new queue for manual syncs in stateReconciliationManager/index.js

  2. update src/services/stateMachineManager/index.js to consume new queue

  3. make serviceRegistry consume new queue as well

  4. move over issueSyncRequestsUntilSynced to src/services/stateMachineManager/index.js and call issueSyncRequestsUntilSynced() in middleware.js

  5. fix issueSyncRequestsUntilSynced to work with the new queue:

Queue#add (on the new manualSyncQueue object from stateReconciliationManager/index.js)

Tests

The primary way this way tested was by running the protocol and client on my dev box, uploading a new track, and checking the logs to make sure the manual sync was performed.

This PR modifies tests for stateReconciliationQueue so that they work with the new api coming from the stateReconciliationManager but doesn't add any new tests for manualSyncQueue

Supporting Screenshot shots

Screen Shot 2022-07-08 at 16 50 42

Screen Shot 2022-07-08 at 16 47 28

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

These changes can be monitoring with Bull health check as well as with new logs.

@jonaylor89 jonaylor89 requested a review from theoilie July 7, 2022 19:46
Copy link
Contributor

@theoilie theoilie left a comment

Choose a reason for hiding this comment

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

just some minor changes and then it should be good to merge assuming it looks fine on the bull dashboard! (/health/bull endpoint)

creator-node/src/middlewares.js Outdated Show resolved Hide resolved
creator-node/src/serviceRegistry.js Outdated Show resolved Hide resolved
@jonaylor89 jonaylor89 marked this pull request as ready for review July 8, 2022 20:58
@jonaylor89 jonaylor89 requested a review from SidSethi July 8, 2022 20:58
@jonaylor89 jonaylor89 changed the title [WIP] Remove manual sync from snapbackSM [CON-225] Remove manual sync from snapbackSM Jul 8, 2022
@theoilie theoilie self-requested a review July 8, 2022 21:13
Copy link
Contributor

@theoilie theoilie left a comment

Choose a reason for hiding this comment

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

couple nits and 🚀

@SidSethi
Copy link
Contributor

SidSethi commented Jul 8, 2022

i'll take a look async, no need to block on my approval! great work

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.

nice!! looks great generally

bit of a deviation from the naming here since there's now stateReconciliationQueue and a manualSyncQueue - will defer to you and @theoilie on standardization here, but smth to think about
i wonder if it would be easier for now to add this as a job type in the stateReconciliationQueue to be consistent with recurring syncs? not sure how i feel about the split states created

per your testing description, want to make sure that it is indeed the new queue you created being used and not the old one

also i don't see any test coverage of this new logic?

for manual testing, would be good to make sure that appropriate configs are in place to never initialize snapback and ensure the correct logic is being selected always

some thoughts from end, will let you and theo sync up

@theoilie
Copy link
Contributor

nice!! looks great generally

bit of a deviation from the naming here since there's now stateReconciliationQueue and a manualSyncQueue - will defer to you and @theoilie on standardization here, but smth to think about i wonder if it would be easier for now to add this as a job type in the stateReconciliationQueue to be consistent with recurring syncs? not sure how i feel about the split states created

per your testing description, want to make sure that it is indeed the new queue you created being used and not the old one

also i don't see any test coverage of this new logic?

for manual testing, would be good to make sure that appropriate configs are in place to never initialize snapback and ensure the correct logic is being selected always

some thoughts from end, will let you and theo sync up

I'd say the disabling snapback side of things is out of scope here as long as this disables snapback's manual sync queue (which it does). I did see Johannes test it manually, and the logs showed that it was processing the sync correctly. @jonaylor89 maybe you could attach a log screenshot showing it process in the correct queue? it would have (queue=manual-sync-queue) as a suffix because you added the JSON queue property to the new manual-sync-queue, but the old manual-sync-queue didn't have that

for state deviation, maybe it would make sense to move toward the one job type per queue model you were thinking of earlier? i.e., breaking up stateReconcliationQueue to recurringSyncQueue and updateReplicaSetQueue

@jonaylor89 jonaylor89 requested a review from SidSethi July 11, 2022 16:42
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.

🚀 once CI passes :)

@jonaylor89 jonaylor89 merged commit 553ad84 into master Jul 11, 2022
@jonaylor89 jonaylor89 deleted the jn-rm-manual-sync branch July 11, 2022 18:23
sliptype pushed a commit that referenced this pull request Sep 10, 2023
[9855fd2] Update locks (#3397) Dylan Jeffers
[aecef5f] [PAY-1144] [PAY-1182] [PAY-1147] DMs: Delete chat, message permissions (#3390) Marcus Pasell
[64ccd9d] Add stylelint to ci (#3373) Dylan Jeffers
[b642a16] [C-2518, C-2523, C-2611] Improve playlist create sagas (#3378) Dylan Jeffers
[5620d53] [PAY-1197] Mobile inbox unavailable modal from profile screen (#3376) Reed
[2ebaef5] [PAY-1248] Initial changes to get ready for upcoming PRs that include track and playlist tiles in DMs (#3391) Saliou Diallo
[c2fabf6] [PAY-1218] Mobile block dms drawer switches block/unblock (#3387) Reed
[0acad29] [C-2575] Match length of related artists user list to preview (#3395) Andrew Mendelsohn
[40525ae] [C-2615] Fix favorite tracks error due to empty track entries on web mobile (#3386) Kyle Shanks
[93a8077] [PAY-1145] DMs: Add InboxUnavailableModal (#3369) Marcus Pasell
[633484d] Update edit playlist flow and components (#3361) Kyle Shanks
[334ea0d] Special case ios safari for stem download (#3385) Raymond Jacobson
[d38238d] [C-2607] Pagination wrapper hooks for audius-query (#3375) Andrew Mendelsohn
[0a332d8] Hotfix: Fix stems downloads on mobile web (#3382) Marcus Pasell
[81da65e] Clean up artist_pick_track_id in APIUser (#3381) Michelle Brier
[64d89ef] Fix lint (#3380) Raymond Jacobson
[59129b4] [C-2614] Fix download stems mobile web (#3379) Raymond Jacobson
[df68727] [PAY-1032][PAY-892] Mobile DMs unread indicator, prefetch chats (#3352) Reed
[4a00fad] [PAY-1151] Handle chat reactions near top of screen on mobile (#3370) Reed
[bff316f] [PAY-1139] Throttle calls to fetchMessages on web scroll (#3372) Michael Piazza
[e723cec] C-2483 Fix queue overshot empty track player bug (#3353) nicoback2
[472a41d] [C-2596] Add disabled option to audius-query hooks (#3367) Andrew Mendelsohn
[f518962] [C-2602] Improve playlist library layout (#3364) Dylan Jeffers
[b3db8fa] Fix debounce on notif reaction (#3362) Raymond Jacobson
[2104b2b] [PAY-1183] Make clicking ChatUser handle/displayname lead to profile (#3368) Michael Piazza
[21cb6fc] DMs: Fix click handler in search user list for users you can't chat (#3358) Marcus Pasell
[24e7d8a] DMs: Update copy, scroll inbox-settings modal (#3359) Marcus Pasell
[e5e8d90] [PAY-941] Fix "1 new messages" unread tag (web) (#3366) Michael Piazza
[cfa1b2a] [C-2603] Fix readonly object error in audius-query reducer (#3365) Andrew Mendelsohn
[978c993] Fix invite reward claimable state on mobile (#3363) Reed
[a174fcc] [C-2556, C-2557] Address AI Attribution QA (#3349) Dylan Jeffers
[3c9b0f1] [PAY-1202] Refactor saved collections fetching (#3337) Randy Schott
[a0bdad5] Get call to action for chat permissions (#3325) Marcus Pasell
[03a2721] DMs: Use the optimistic unread count if applicable (#3354) Marcus Pasell
[8b99c2b] [C-2550] Left-nav fixes and improvements (#3357) Dylan Jeffers
[e7b0aab] [PAY-1215] Fix create new message crash (#3356) Reed
[6e7ece9] [PAY-1196] Mobile dms search users empty state (#3355) Reed
[315ae4f] [PAY-1219] Fix mobile chat reactions popup message getting cut off (#3342) Reed
@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.

4 participants