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

feat: add background_updates endpoints #24

Merged
merged 9 commits into from
Jan 26, 2024

Conversation

hanadi92
Copy link
Contributor

@hanadi92 hanadi92 commented Jan 19, 2024

Following the docs from administration:

  • Status of background updates GET /_synapse/admin/v1/background_updates/status
  • Trigger background updates POST /_synapse/admin/v1/background_updates/enabled
  • Start a job POST /_synapse/admin/v1/background_updates/start_job

pub name: String,

/// Total number of processed "items"
pub total_item_count: i64,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: one of the background updates is syncing the user directory, which in huge servers could reach to billions. At least, I've seen it reaching a billion. Feel free to correct me here.

Copy link
Member

Choose a reason for hiding this comment

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

If this was part of the spec, I would be a bit concerned about this (especially if the value range wasn't called out in the spec). However, it's just the Synapse Admin API which is not as rigorously specified as the official /_matrix/ endpoints so going by what makes sense realistically is perfectly fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set it to i32. I haven't found any specs to how high this can get.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, why? The previous type made more sense to me with the context you provided 😅

Or actually, why allow negative values? I think the two most sensible types here would be u64 and js_int::UInt (which would be used if this was in ruma itself).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Darn it. Yes, I should have used unsigned here.
I thought it's good to allocate the least memory possible since there was no documentation. You are right, my context should have been my documentation for it. 👍

@hanadi92 hanadi92 force-pushed the feat-background-updates-apis branch 3 times, most recently from 7e868d3 to 2b563bb Compare January 20, 2024 21:14
/// Valid values are:
/// populate_stats_process_rooms
/// regenerate_directory
pub job_name: String,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

question: wondering if we should struct the valid options here. but could be an over kill.

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable, just define an enum with #[serde(rename_all = "snake_case")]. Ruma also has a derive StringEnum if you want to allow extra custom values while still having enum variants for the well-known strings (see other uses of that derive macro).

@hanadi92 hanadi92 force-pushed the feat-background-updates-apis branch from 479196d to e7694fd Compare January 20, 2024 21:25
@hanadi92 hanadi92 marked this pull request as ready for review January 21, 2024 10:45
@hanadi92 hanadi92 force-pushed the feat-background-updates-apis branch from 531b647 to 4d05290 Compare January 22, 2024 14:33
@hanadi92 hanadi92 requested a review from jplatte January 26, 2024 19:16
src/background_updates/run/v1.rs Outdated Show resolved Hide resolved
src/background_updates/status/v1.rs Outdated Show resolved Hide resolved
src/background_updates/status/v1.rs Outdated Show resolved Hide resolved
src/background_updates/status/v1.rs Outdated Show resolved Hide resolved
src/background_updates/status/v1.rs Outdated Show resolved Hide resolved
src/background_updates/status/v1.rs Outdated Show resolved Hide resolved
src/background_updates/enabled/v1.rs Outdated Show resolved Hide resolved
src/background_updates/run/v1.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@jplatte jplatte enabled auto-merge (squash) January 26, 2024 20:28
@jplatte jplatte merged commit 470f06d into ruma:main Jan 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants