Skip to content

Used crate ID for background jobs #11455

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

YohDeadfall
Copy link

@YohDeadfall YohDeadfall commented Jun 27, 2025

Should the jobs store crate names for logging purposes? Or should they just show the provided ID instead?

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

Fixes #11349

I will remove that part of the issue description, since a) this is only covering one of the problematic jobs, and b) this job will also need a follow-up PR to migrate away from Option<i32> to i32.

Comment on lines 47 to 56
let version_updates = load_version_updates(name, &mut conn).await?;
let version_updates = load_version_updates(crate_id, &mut conn).await?;
Copy link
Member

Choose a reason for hiding this comment

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

it's probably easiest to keep using name for now and migrate over to crate_id in the next PR when this PR here has been deployed to production. That way we won't need the load_crate_id() fn at all :)

Copy link
Author

Choose a reason for hiding this comment

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

So, the first step is only for adding identifiers to the jobs without changing how they work, right?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, exactly. that should make it easier to migrate them.

@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Jun 27, 2025
@YohDeadfall
Copy link
Author

I will remove that part of the issue description

That's why it's only a draft. I have some questions and would like to deal with them first on a single job, and then do the same for the rest.

@YohDeadfall
Copy link
Author

A side question. Does it makes sense to provide configuration files for the analyzer as it's done in Miri and Rust itself? An example: rust-lang/miri@d835aee.

@Turbo87
Copy link
Member

Turbo87 commented Jun 27, 2025

Does it makes sense to provide configuration files for the analyzer as it's done in Miri and Rust itself?

idk, I use the JetBrains IDEs, so I don't use rust-analyzer 😅

@YohDeadfall
Copy link
Author

If I'm not wrong then it's still rust-analyzer based, but it can be provided by the IDE itself. Anyway, we can live without adding these configs as no one complained except me, and I fixed it locally.

@YohDeadfall
Copy link
Author

So, DeleteCrateFromStorage uses name only as it never fetches something by it. Meanwhile, it's created when there's no crate exists:

info!("{name}: Skipped missing crate");
};
info!("{name}: Enqueuing background jobs…");
let git_index_job = jobs::SyncToGitIndex::new(name);
let sparse_index_job = jobs::SyncToSparseIndex::new(name);
let delete_from_storage_job = jobs::DeleteCrateFromStorage::new(name.into());

Is it a bug or a feature?

@YohDeadfall YohDeadfall changed the title Used crate ID for SyncCrateFeed Used crate ID for background jobs Jun 28, 2025
@YohDeadfall YohDeadfall marked this pull request as ready for review June 28, 2025 00:21
@YohDeadfall YohDeadfall force-pushed the crate-id-based-jobs branch from a54fba2 to c8cdb58 Compare June 28, 2025 00:22
@Turbo87
Copy link
Member

Turbo87 commented Jun 28, 2025

DeleteCrateFromStorage uses name only as it never fetches something by it.

hmm, I guess in that case we should probably keep using the name since we can't delete by ID anyway.

Meanwhile, it's created when there's no crate exists. Is it a bug or a feature?

that is actually intentional. we had a couple of cases in the past where a crate was deleted from the database by our support team but wasn't removed from S3 storage. this implementation allowed us to remove it from storage even though it didn't exist in the database anymore.

@@ -1,2 +1,3 @@
[toolchain]
channel = "1.88.0"
components = ["rust-analyzer"]
Copy link
Author

Choose a reason for hiding this comment

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

That's unintentional and will be removed. It fixes the issue with helix at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants