Skip to content

Conversation

reivilibre
Copy link
Contributor

Commit-by-commit suggested

@reivilibre reivilibre requested a review from sandhose January 29, 2025 14:23
Copy link

cloudflare-workers-and-pages bot commented Jan 29, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6b02497
Status: ✅  Deploy successful!
Preview URL: https://6f583dfb.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://rei-syn2mas-7-external-ids.matrix-authentication-service-docs.pages.dev

View logs

@reivilibre reivilibre force-pushed the rei/syn2mas_7_external_ids branch from 704ddb7 to 6b02497 Compare January 29, 2025 14:44
#[allow(clippy::missing_panics_doc)] // not a real panic
#[tracing::instrument(skip_all, level = Level::DEBUG)]
pub async fn write_users(&mut self, users: Vec<MasNewUser>) -> Result<(), Error> {
pub fn write_users(&mut self, users: Vec<MasNewUser>) -> BoxFuture<'_, Result<(), Error>> {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning with boxing here, and why not -> impl Future?

Comment on lines +726 to +745
let mut link_ids: Vec<Uuid> = Vec::with_capacity(links.len());
let mut user_ids: Vec<Uuid> = Vec::with_capacity(links.len());
let mut upstream_provider_ids: Vec<Uuid> = Vec::with_capacity(links.len());
let mut subjects: Vec<String> = Vec::with_capacity(links.len());
let mut created_ats: Vec<DateTime<Utc>> = Vec::with_capacity(links.len());

for MasNewUpstreamOauthLink {
link_id,
user_id,
upstream_provider_id,
subject,
created_at,
} in links
{
link_ids.push(link_id);
user_ids.push(user_id);
upstream_provider_ids.push(upstream_provider_id);
subjects.push(subject);
created_ats.push(created_at);
}
Copy link
Member

Choose a reason for hiding this comment

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

itertools::multiunzip is a thing, not sure it would make that better though so 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mrmm, it's hard to find a tool that works here other than doing it the boring way. I don't see that much point in destructuring to a tuple. Maybe it's a little better, but not by much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

posterity note: the crates soa_derive and columnar look like they could be promising for this sort of thing, but it doesn't seem worth adding a new dep (+ proc_macro + compile time) for this when we only have a handful of these to do

Comment on lines +906 to +907
type WriteBufferFlusher<'conn, T> =
for<'a> fn(&'a mut MasWriter<'conn>, Vec<T>) -> BoxFuture<'a, Result<(), Error>>;
Copy link
Member

Choose a reason for hiding this comment

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

Riiight, all the box future make sense now :)

rng: &mut impl RngCore,
) -> Result<UsersMigrated, Error> {
let mut write_buffer = MasUserWriteBuffer::new(mas);
let mut user_buffer = MasWriteBuffer::new(MasWriter::write_users);
Copy link
Member

Choose a reason for hiding this comment

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

not sure how I feel about passing function pointers instead of using traits… I guess that works, not sure how it would look like with traits

@reivilibre reivilibre merged commit fec4efd into main Jan 30, 2025
21 checks passed
@reivilibre reivilibre deleted the rei/syn2mas_7_external_ids branch January 30, 2025 10:34
@sandhose sandhose added A-Migration Related to the migration tooling T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. labels Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Migration Related to the migration tooling T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants