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

sync mechanism for modifications - baseline #1028

Merged
merged 10 commits into from
Feb 17, 2025

Conversation

eaypek-tfh
Copy link
Collaborator

@eaypek-tfh eaypek-tfh commented Feb 14, 2025

Change

  • Create the modifications table which will store reauth and deletion requests for now. We can expand it in the future with uniqueness as well.
  • The next PR will implement the sync mechanism based on collected data.
  • Store each deletion/reauth request when they're received with status=IN_PROGRESS and persisted=false.
  • After the batch, update each deletion request with status=COMPLETED and persisted=true.
  • After the batch, update each reauth request with status=COMPLETED and persisted={success_result}.

Side Changes

  • Introduce a max_deletions_per_batch config so that we can safely define a lookback size for modifications sync.
    • Note: Currently, deletion processor cron in signup-service sends all the deletion requests for which the cooldown has ended. Since deletions are not counted towards the batch size, we can potentially executed 100s of them. We'll change it so that it can send the same max_deletions_per_batch per cron execution.
  • Do not allow a serial id to have multiple modifications in a single batch.

Links

@eaypek-tfh eaypek-tfh changed the title Pop 2058/sync mechanism for modifications sync mechanism for modifications - baseline Feb 14, 2025
@eaypek-tfh eaypek-tfh force-pushed the POP-2058/sync-mechanism-for-modifications branch from c4aaa8a to abc8ae7 Compare February 14, 2025 14:20
Comment on lines +532 to +549
let ids: Vec<i64> = modifications.iter().map(|m| m.id).collect();
let statuses: Vec<String> = modifications.iter().map(|m| m.status.clone()).collect();
let persisteds: Vec<bool> = modifications.iter().map(|m| m.persisted).collect();

sqlx::query(
r#"
UPDATE modifications
SET status = data.status,
persisted = data.persisted
FROM (
SELECT
unnest($1::bigint[]) as id,
unnest($2::text[]) as status,
unnest($3::bool[]) as persisted
) as data
WHERE modifications.id = data.id
"#,
)
Copy link
Collaborator Author

@eaypek-tfh eaypek-tfh Feb 14, 2025

Choose a reason for hiding this comment

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

needed to use unnest to get the tests working. strangely, query builder did not work. i get this error before this commit

let me know if you know of a way to avoid the unnest if you think it looks ugly 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be the recommended way in sqlx doc. How can I bind an array…

@eaypek-tfh eaypek-tfh force-pushed the POP-2058/sync-mechanism-for-modifications branch from 9f86303 to 7732134 Compare February 14, 2025 14:42
@eaypek-tfh eaypek-tfh requested a review from bgillesp February 14, 2025 15:39
Copy link
Collaborator

@naure naure left a comment

Choose a reason for hiding this comment

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

Looks like a good direction to me.

iris-mpc-common/src/helpers/sync.rs Outdated Show resolved Hide resolved
iris-mpc-cpu/src/execution/hawk_main.rs Outdated Show resolved Hide resolved
}) = rx.recv().await
{
let _modifications = modifications;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should stay in-sync and do the same as server.rs.

(For sure this is a drawback of the current state, need refactoring.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I intentionally skipped any changes in hawk server because it's just duplicate of gpu server. i think this can be handled together with refactoring. i think any effort i put there now would probably be deleted during refactoring

Comment on lines +532 to +549
let ids: Vec<i64> = modifications.iter().map(|m| m.id).collect();
let statuses: Vec<String> = modifications.iter().map(|m| m.status.clone()).collect();
let persisteds: Vec<bool> = modifications.iter().map(|m| m.persisted).collect();

sqlx::query(
r#"
UPDATE modifications
SET status = data.status,
persisted = data.persisted
FROM (
SELECT
unnest($1::bigint[]) as id,
unnest($2::text[]) as status,
unnest($3::bool[]) as persisted
) as data
WHERE modifications.id = data.id
"#,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be the recommended way in sqlx doc. How can I bind an array…

@eaypek-tfh eaypek-tfh merged commit 6d93d5c into main Feb 17, 2025
12 checks passed
@eaypek-tfh eaypek-tfh deleted the POP-2058/sync-mechanism-for-modifications branch February 17, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants