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

state: Allow setting explicit priority for any job #962

Closed
radeksimko opened this issue Jun 22, 2022 · 1 comment · Fixed by #977
Closed

state: Allow setting explicit priority for any job #962

radeksimko opened this issue Jun 22, 2022 · 1 comment · Fixed by #977
Assignees
Labels
bug Something isn't working
Milestone

Comments

@radeksimko
Copy link
Member

radeksimko commented Jun 22, 2022

Background

Currently, jobs have implied priority based on whether files within the directory which the job affects

func (js *JobStore) AwaitNextJob(ctx context.Context, openDir bool) (job.ID, job.Job, error) {
// Locking is needed if same query is executed in multiple threads,
// i.e. this method is called at the same time from different threads, at
// which point txn.FirstWatch would return the same job to more than
// one thread and we would then end up executing it more than once.
if openDir {
js.nextJobOpenDirMu.Lock()
defer js.nextJobOpenDirMu.Unlock()
} else {
js.nextJobClosedDirMu.Lock()
defer js.nextJobClosedDirMu.Unlock()
}
return js.awaitNextJob(ctx, openDir)
}
func (js *JobStore) awaitNextJob(ctx context.Context, openDir bool) (job.ID, job.Job, error) {
txn := js.db.Txn(false)
wCh, obj, err := txn.FirstWatch(js.tableName, "is_dir_open_state", openDir, StateQueued)
if err != nil {
return "", job.Job{}, err
}
if obj == nil {
select {
case <-wCh:
case <-ctx.Done():
return "", job.Job{}, ctx.Err()
}
return js.awaitNextJob(ctx, openDir)
}
sJob := obj.(*ScheduledJob)
err = js.markJobAsRunning(sJob)
if err != nil {
// Although we hold a write db-wide lock when marking job as running
// we may still end up passing the same job from the above read-only
// transaction, which does *not* hold a db-wide lock.
//
// Instead of adding more sync primitives here we simply retry.
if errors.Is(err, jobAlreadyRunning{ID: sJob.ID}) || errors.Is(err, jobNotFound{ID: sJob.ID}) {
js.logger.Printf("retrying next job: %s", err)
return js.awaitNextJob(ctx, openDir)
}
return "", job.Job{}, err
}
js.logger.Printf("JOBS: Dispatching next job %q: %q for %q", sJob.ID, sJob.Type, sJob.Dir)
return sJob.ID, sJob.Job, nil
}

This is a reasonable design for most jobs, but #924 introduced a new job, which is scheduled for open files, primarily because it relies on up-to-date AST.

Running that job in the main indexer is rather undesirable side effect of the main indexer taking any jobs concerning open files. It's not desirable because unlike other jobs, this one has a higher probability of failing and/or taking longer to finish, potentially delaying other more important tasks.

Proposal

Introduce more 1st class job priorities, such that the priority can be set during scheduling, instead of implied based on the file/directory.

That way we could still schedule GetModuleDataFromRegistry when the concerning file is open, but run it with lower priority on the background without blocking e.g. parsing or reference collections.


Example log where the request to the Registry times out: https://gist.github.com/radeksimko/20bbb39273caf8d1c557f56bf755f827

@radeksimko radeksimko added the bug Something isn't working label Jun 22, 2022
@radeksimko radeksimko added this to the v0.29.0 milestone Jun 22, 2022
@radeksimko radeksimko self-assigned this Jun 22, 2022
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant