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.

Sign up for GitHub

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

Jump to bottom

CON-238 - Divergent state PR#4 - Primary Sync from Secondary #3399

Closed
wants to merge 10 commits into from
Closed

CON-238 - Divergent state PR#4 - Primary Sync from Secondary #3399

wants to merge 10 commits into from

Conversation

SidSethi
Copy link
Contributor

@SidSethi SidSethi commented Jul 7, 2022

Description

Tests

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

@SidSethi SidSethi changed the title primary sync from secondary CON-238 - Divergent state PR#4 - Primary Sync from Secondary Jul 7, 2022
dmanjunath
dmanjunath reviewed Jul 7, 2022
View reviewed changes
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

didn't do a full PR review yet, just looked at creator-node/src/services/sync/primarySyncFromSecondary.js. logic looks solid. few things i want to just post so i don't forget

  • this is only run in the state machine, not part of user write flow
  • doesn't impact secondarySyncFromPrimary (old processSync, thx for renaming) called by secondaries or the write quorum work that theo's doing
  • because we call forceResync=true after the primary finishes, the secondaries will get wiped and resynced and all should be good

creator-node/src/services/sync/primarySyncFromSecondary.js Outdated Show resolved Hide resolved
creator-node/src/redis.js Show resolved Hide resolved
creator-node/src/redis.js Show resolved Hide resolved
@SidSethi SidSethi mentioned this pull request Jul 7, 2022
Closed
theoilie
theoilie reviewed Jul 8, 2022
View reviewed changes
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.

good stuff!! I had some questions and saw a few things that I'm sure are just there for testing but made GH comments to make sure they don't slip through the cracks if a future re-review isn't as thorough

creator-node/src/services/prometheusMonitoring/prometheus.constants.js Outdated Show resolved Hide resolved
creator-node/src/services/stateMachineManager/stateMonitoring/findSyncRequests.jobProcessor.js Show resolved Hide resolved
creator-node/src/services/stateMachineManager/stateMonitoring/findSyncRequests.jobProcessor.js Show resolved Hide resolved
creator-node/src/services/stateMachineManager/stateMonitoring/findSyncRequests.jobProcessor.js Outdated Show resolved Hide resolved
...r-node/src/services/stateMachineManager/stateReconciliation/issueSyncRequest.jobProcessor.js Show resolved Hide resolved
creator-node/test/findSyncRequests.jobProcessor.test.js Outdated
Comment on lines 898 to 902
it.skip('test for when _findSyncsforUser returns syncReqsToEnqueue and duplicateSyncReqs')

it.skip('test for when _findSyncsforUser returns syncReqsToEnqueue and errors')

it.skip('test with multiple users and outcomes')
Copy link
Contributor

Choose a reason for hiding this comment

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

just flagging this so it doesn't slip through since I'm guessing it's a TODO

creator-node/test/issueSyncRequest.jobProcessor.test.js Show resolved Hide resolved
creator-node/test/issueSyncRequest.jobProcessor.test.js Show resolved Hide resolved
creator-node/test/issueSyncRequest.jobProcessor.test.js Outdated Show resolved Hide resolved
creator-node/test/sync.test.js Show resolved Hide resolved
@SidSethi
Copy link
Contributor Author

  • this is only run in the state machine, not part of user write flow
  • doesn't impact secondarySyncFromPrimary (old processSync, thx for renaming) called by secondaries or the write quorum work that theo's doing
  • because we call forceResync=true after the primary finishes, the secondaries will get wiped and resynced and all should be good

yep all accurate

@SidSethi SidSethi mentioned this pull request Jul 12, 2022
Merged
@SidSethi
Copy link
Contributor Author

closing this since its been split across multiple other PRs: #3415, #3425, #3426

@SidSethi SidSethi closed this Jul 12, 2022
@SidSethi SidSethi deleted the ss-dsr-primarysyncfromsecondary branch July 12, 2022 17:07
sliptype pushed a commit that referenced this pull request Sep 10, 2023
@dharit-tan
[PAY-1242] Mobile chats copy message button (#3399)
0e1736b
sliptype pushed a commit that referenced this pull request Sep 10, 2023
v1.5.26
7c6b148 
[2c08c63] [C-2653] Add shallow argument to audius-query hooks (#3450) Andrew Mendelsohn
[0e5d1b3] [PAY-1247] Cap height of chat textinput on mobile (#3447) Reed
[efaa425] [PAY-1243] Mobile chat header touchable feedback (#3448) Reed
[3980e00] [C-2647] Use entire cache to denormalize data (#3449) Andrew Mendelsohn
[1395822] [PAY-1150] Disable scrolling when chat reaction popup is active (#3445) Reed
[f4ad5f7] [C-2592] Fix edit playlist (#3443) Kyle Shanks
[875054a] Fix mobile native change comment C-2657 (#3444) nicoback2
[63b5ab4] Bump SDK version and change walletapi to auth (#3440) nicoback2
[1611465] DMs: Use unblock/block confirmation modals on profile (#3437) Marcus Pasell
[179dda6] [PAY-1142] DMs: Update chat tail (#3438) Marcus Pasell
[95f1843] [PAY-946] Handle signed out users across DMs (#3423) Michael Piazza
[682eebb] [C-2649] Refactor desktop collection header (#3441) Dylan Jeffers
[7310fae] [PAY-800][PAY-1249][PAY-1250] Display track and playlist tiles in web chats - Part 1 (#3377) Saliou Diallo
[689fbcb] [PAY-1274] DMs: Unsticky inbox unavailable message (#3435) Marcus Pasell
[0212e19] Fix unread count incorrect on mobile chats (#3430) Reed
[6743352] Update legacy playlist screens to use legacy inputs (#3436) Kyle Shanks
[fb25e74] [CON-678] v2 user profile edit (#3431) Theo Ilie
[abf827f] [PAY-1255] [PAY-1245] DMs: Add confirmation modals (#3424) Marcus Pasell
[536d3c6] Make mobile chat inbox unavailable message unsticky (#3434) Reed
[f11f266] [PAY-1273] Fix scrolling issues (#3433) Michael Piazza
[26ec389] [PAY-1258] Add toast when chat messages are copied (#3421) Randy Schott
[9ba1956] [PAY-1251] [PAY-1188] DMs: Add error message to failed message sends (#3428) Marcus Pasell
[14034ae] [C-2597][C-2651] Add audius-query usage docs (#3427) Andrew Mendelsohn
[16deebc] [C-2508] Update playback positions to be tracked per user id (#3374) Kyle Shanks
[2d6b278] [C-2644] Fix issue where new playlist drag-drop not persisting (#3422) Dylan Jeffers
[a9c2c5e] [PAY-1212] [PAY-1146] Reactions and message UI polish (#3414) Michael Piazza
[ab8bd6b] Type fix for hook options (#3426) Andrew Mendelsohn
[d167810] [PAY-1272] Make desktop DM text copyable (#3425) Michael Piazza
[7ba5973] [PAY-925] Adds unread message icons on web (#3418) Randy Schott
[645f0a5] [PAY-935] Fix ChatHeader issue (#3419) Michael Piazza
[cc1c6e8] Update mobile playlist create flows to conform with new designs (#3416) Kyle Shanks
[9701930] [PAY-1260] New chat tails on mobile (#3420) Reed
[dd2873e] [PAY-1241] Skeleton for mobile chat list screen (#3393) Reed
[a373e62] [PAY-1253] Mobile message resend button (#3413) Reed
[ddc5540] Fix type error track availability modal C-2632 (#3415) nicoback2
[19929b9] [C-2601 C-2627 C-2628 C-2638] Playlist library QA (#3411) Dylan Jeffers
[5a7f61a] [C-2468] Add desktop create flow (#3409) Dylan Jeffers
[b0c1583] Mark chat as read on ChatScreen entry (#3410) Reed
[6e120cd] [PAY-1222] Fix unread messages count (#3407) Michael Piazza
[95dd306] [C-2299] Fix NowPlayingDrawer navigation deduping bug (#3405) Sebastian Klingler
[01fcab4] [PAY-1195] Mobile chat screen unavailable states (#3402) Reed
[67b1788] [C-2598] Add types for audius-query hooks (#3406) Andrew Mendelsohn
[8bf784a] Increase space between chat reaction and copy message button (#3403) Reed
[2cba881] Fix upload collection C-2630 (#3404) nicoback2
[aee7653] [PAY-1242] Mobile chats copy message button (#3399) Reed
[01ec1fb] [PAY-1220] Fix chat receive bug (#3400) Michael Piazza
[0ad899c] Add useAuthenticatedCallback (#3392) Dylan Jeffers
[412ca92] Improve popup-menu a11y (#3394) Dylan Jeffers
[ecdfb7e] [PAY-1136][PAY-1129] Add infinite loading to albums on desktop / mobile web (#3371) Randy Schott
[35678eb] [PAY-1028] Fix mobile chat actions drawer touch feedback (#3388) 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
Reviewers

@theoilie theoilie theoilie left review comments

@dmanjunath dmanjunath dmanjunath left review comments

Assignees
No one assigned
Labels
size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SidSethi @dmanjunath @theoilie