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

Run git gc periodically on the crates.io index (#778) #956

Closed
wants to merge 143 commits into from
Closed

Run git gc periodically on the crates.io index (#778) #956

wants to merge 143 commits into from

Conversation

ohaddahan
Copy link
Contributor

@ohaddahan ohaddahan commented Aug 11, 2020

Forking git gc periodically from start_registry_watcher.

Closes #778.

.gitignore Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/utils/daemon.rs Outdated Show resolved Hide resolved
src/utils/daemon.rs Outdated Show resolved Hide resolved
src/utils/daemon.rs Outdated Show resolved Hide resolved
ohaddahan and others added 9 commits August 11, 2020 23:22
Co-authored-by: Chase Wilson <buckshot1233@gmail.com>
Co-authored-by: Chase Wilson <buckshot1233@gmail.com>
Co-authored-by: Chase Wilson <buckshot1233@gmail.com>
Co-authored-by: Chase Wilson <buckshot1233@gmail.com>
Co-authored-by: Chase Wilson <buckshot1233@gmail.com>
use std::time::{Duration, Instant};

fn run_git_gc() {
let gc = Command::new("git").args(&["gc", "--auto"]).output();
Copy link
Member

Choose a reason for hiding this comment

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

This needs the path to the repo to clean. I think it'd make most sense to move this to a method on index::Index since that's what needs cleaning and has the path available. You can then access it via docbuilder.index to call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nemo157 inside what method?

How about RustwideBuilder::build_package ? Doesn't it run in the same location as the git repo of the crate?

Copy link
Member

Choose a reason for hiding this comment

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

A new method, probably just keep this name for it run_git_gc.

I have vague ideas of eventually removing the index to queue handling from docbuilder completely, it isn't really related to building the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nemo157 what I meant was, where in the Index flow to add it.

I checked now and the Index is called from RustwideBuilder which is called from the Queue.

So maybe add last_gc_run property on the Queue and call run_git_gc conditionally from inside build_package? Something along these lines?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking just keep calling it from here, this is the place that causes us to create new packfiles so should be responsible for knowing when to clean them up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nemo157 what is the location you refer to by calling it from here?

Copy link
Member

Choose a reason for hiding this comment

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

I assume he meant daemon.rs. So fn run_git_gc gets moved to Index, but still gets called from start_registry_watcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jyn514 @Nemo157 updated, let me know if that what you meant 😊

src/config.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added A-admin Area: Administration of the production docs.rs server S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Aug 12, 2020
src/web/metrics.rs Outdated Show resolved Hide resolved
src/web/metrics.rs Outdated Show resolved Hide resolved
src/web/metrics.rs Outdated Show resolved Hide resolved
… and CURRENTLY_RUNNING_THREADS declaration match the usage, "target_os = linux"
git_hooks/pre-commit Show resolved Hide resolved
git_hooks/pre-commit Show resolved Hide resolved
git_hooks/pre-commit Show resolved Hide resolved
git_hooks/pre-commit Show resolved Hide resolved
use std::time::{Duration, Instant};

fn run_git_gc() {
let gc = Command::new("git").args(&["gc", "--auto"]).output();
Copy link
Member

Choose a reason for hiding this comment

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

I assume he meant daemon.rs. So fn run_git_gc gets moved to Index, but still gets called from start_registry_watcher.

@Kixiron
Copy link
Member

Kixiron commented Aug 13, 2020

@ohaddahan Could you push your commits so we can see the changes?

@ohaddahan
Copy link
Contributor Author

@Kixiron ops sorry, forgot to merge my fork branch 😊

@jyn514
Copy link
Member

jyn514 commented Aug 13, 2020

@ohaddahan Hi, your commits messages say 'rebase' but rebases do not have commits, they apply the original commits on top of the original branch. As it stands this has 143 commits and a 900 line diff. Can you please rebase using something like the following:

git checkout master
git fetch origin
git rebase -i origin/master

Since you have several merge commits there will be many conflicts, so you should drop all commits that are not related to this PR. Alternatively, it might be easier to start over on a new branch:

git checkout origin/master
git checkout --branch git-gc
git cherry-pick all the relevent commit hashes

However, as-is, this is very difficult to review.

@jyn514
Copy link
Member

jyn514 commented Aug 13, 2020

If you want help feel free to join the discord channel: https://discord.gg/f7mTXPW

@ohaddahan
Copy link
Contributor Author

@jyn514 yeah my fork went down the drain, I'll clean it up.

@jyn514
Copy link
Member

jyn514 commented Aug 15, 2020

Closing in favor of #975

@jyn514 jyn514 closed this Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-admin Area: Administration of the production docs.rs server S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run git gc periodically on the crates.io index
6 participants