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

De-dupe sync tasks #1346

Merged
merged 11 commits into from
Mar 30, 2021
Merged

De-dupe sync tasks #1346

merged 11 commits into from
Mar 30, 2021

Conversation

SidSethi
Copy link
Contributor

@SidSethi SidSethi commented Mar 24, 2021

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?

Spec in Notion

Problem: often see multiple syncs in queue for same user/secondary - this is redundant and takes spots away from other syncs
Solution: Bull provides no helpful API, so implemented an in-memory map to record progress syncs and prevent duplicate syncs from being enqueued.

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 ❗

Snapback integration & system tests passing

❗ 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.

@@ -141,15 +141,32 @@ async function ensureStorageMiddleware (req, res, next) {
* @dev - TODO move this out of middlewares to Services layer
*/
async function triggerSecondarySyncs (req) {
if (config.get('isUserMetadataNode') || config.get('snapbackDevModeEnabled')) return
const { snapbackSM } = serviceRegistry
Copy link
Contributor Author

@SidSethi SidSethi Mar 24, 2021

Choose a reason for hiding this comment

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

every change in this file is just code cleanup, except for line 165 (the enqueueSync() call

primaryEndpoint,
secondaryEndpoint,
userWallet,
primaryClockValue,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing primaryClockValue from job inputs since it is unrelated and it also makes it impossible to dedupe syncs effectively

@SidSethi SidSethi changed the title Ss aud 406 dedupe syncs De-dupe sync tasks Mar 24, 2021
@SidSethi SidSethi added bug Something isn't working content-node Content Node (previously known as Creator Node) labels Mar 24, 2021
@SidSethi SidSethi marked this pull request as ready for review March 24, 2021 20:05
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 PR is amazing 🙏

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

Looks good

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

Merging despite mad-dog failure as it is the same 404 error and already tested on staging

@SidSethi SidSethi merged commit 80f1ded into master Mar 30, 2021
@SidSethi SidSethi deleted the ss-aud-406-dedupe-syncs branch March 30, 2021 17:40
dmanjunath pushed a commit that referenced this pull request Apr 2, 2021
@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
bug Something isn't working content-node Content Node (previously known as Creator Node)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants