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

Split Snapback Sync queue into separate queues for Manual and Recurring #1308

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

SidSethi
Copy link
Contributor

Description

What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?

Notion doc

Problem: manual (high pri) syncs sometimes are blocked on recurring (low pri) syncs - this can happen if the queue has active tasks up to max concurrency, many of which are likely to be recurring syncs. This is suboptimal since manual syncs block user interaction and should never be blocked by background syncs.

Solution: create two queues: ManualSyncQueue and RecurringSyncQueue, each with their own max concurrency.

Tests

List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗

  1. Updated CN snapback unit and integration tests to reflect updated behavior
  2. Confirmed mad-dog snapback test works

❗ Reminder 💡❗:
If this PR touches a critical flow (such as Indexing, Uploads, Gateway or the Filesystem), make sure to add the requires-special-attention label. Add relevant labels as necessary.

@SidSethi SidSethi added the content-node Content Node (previously known as Creator Node) label Mar 16, 2021
dmanjunath
dmanjunath previously approved these changes Mar 16, 2021
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.

This looks good. Looks pretty straightforward. Had one q (possibly no-op) and mad dog is failing. once those are addressed I can re-approve

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

This looks good. Looks pretty straightforward. Had one q (possibly no-op) and mad dog is failing. once those are addressed I can re-approve

for mad-dog failure, it only failed on the merge commit and succeeded on each of 3 previous commits. so if anything failed it was from master - i'm re-running now tho and will see

hareeshnagaraj
hareeshnagaraj previously approved these changes Mar 16, 2021
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

Have some questions, no blockers

creator-node/src/snapbackSM.js Outdated Show resolved Hide resolved
creator-node/test/snapbackSM.test.js Outdated Show resolved Hide resolved
creator-node/src/snapbackSM.js Show resolved Hide resolved
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.

Amazing!

@SidSethi SidSethi merged commit 1b1ab15 into master Mar 17, 2021
@SidSethi SidSethi deleted the ss-aud-406-nodesync-improvements branch March 17, 2021 18:13
@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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants