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(ai-help): delete ai history after six months #437

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

argl
Copy link
Contributor

@argl argl commented Mar 4, 2024

Adds a new admin endpoint that deletes all ai help history entries form the database that are more that 6 months old.

(MP-712)

@argl argl marked this pull request as ready for review March 4, 2024 10:32
@argl argl requested a review from a team as a code owner March 4, 2024 10:32
src/db/v2/mod.rs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what v2 under db refer to?

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 do not know, I put it there for consistency w.r.t. the bcd updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After talking to @fiji-flo, I will move things around a bit.

@caugner caugner requested review from fiji-flo and a team and removed request for a team March 4, 2024 11:34
@argl argl requested a review from caugner March 5, 2024 12:50
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just some thoughts.

Comment on lines +175 to +176
let mut retry = 0;
const MAX_RETRIES: u32 = 40;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why you use u32, since u8 would be sufficient for both, and the difference is probably negligible here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, u8 would suffice here. It is test code though, so I we can be a bit more relaxed about this.

Comment on lines +178 to +192
loop {
records = ai_help_history::table
.filter(ai_help_history::user_id.eq(1))
.select(ai_help_history::updated_at)
.get_results(&mut conn)?;
if records.len() == 1 {
break;
}

actix_rt::time::sleep(Duration::from_millis(5)).await;
retry += 1;
if retry > MAX_RETRIES {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be made more readable by using the retry crate, and we could reuse it to stabilize those flaky tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, probably. Lets look into retry crate next time we have flakyness issues.

Comment on lines +38 to +40
if let Err(e) = do_delete_old_ai_history(pool).await {
error!("{}", e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering out loud if this would be "more" idiomatic:

Suggested change
if let Err(e) = do_delete_old_ai_history(pool).await {
error!("{}", e);
}
do_delete_old_ai_history(pool)
.await
.map_err(|e| error!("{}", e))
.ok();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it looks more rustful.

@argl argl merged commit 92541a5 into main Mar 18, 2024
3 checks passed
@argl argl deleted the MP-712-six-month-history-deletion branch March 18, 2024 16:48
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