Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[fix] perms jobs not being scheduled if there is not already a job#54278

Merged
kopancek merged 3 commits into
mainfrom
milan/fix_scheduled_perms_sync_queues
Jun 27, 2023
Merged

[fix] perms jobs not being scheduled if there is not already a job#54278
kopancek merged 3 commits into
mainfrom
milan/fix_scheduled_perms_sync_queues

Conversation

@kopancek

Copy link
Copy Markdown
Contributor

Previously, the usersWithOldestPermsQuery and reposWithOldestPermsQuery relied solely on the permission_sync_jobs table. If there was no row for the user or repo, this query would not return that user anymore.

This resulted in users/repos never being scheduled for a permission sync in cases when a job was not created previously (or jobs table was truncated).

Attempted to fix the queries by including those users/repos that do not have any record in the jobs table and putting these users/repos at the top of the queue.

Test plan

unit test modified, tested locally

Previously, the `usersWithOldestPermsQuery` and `reposWithOldestPermsQuery`
relied solely on the permission_sync_jobs table. If there was no row for the
user or repo, this query would not return that user anymore.

This resulted in users/repos never being scheduled for a permission sync in
cases when a job was not created previously (or jobs table was truncated).

Attempted to fix the queries by including those users/repos that do not
have any record in the jobs table and putting these users/repos at the top
of the queue.
@cla-bot cla-bot Bot added the cla-signed label Jun 27, 2023
@kopancek kopancek requested review from a team June 27, 2023 12:06
@sourcegraph-bot

sourcegraph-bot commented Jun 27, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 98e5049...c3d0fb6.

Notify File(s)
@efritz enterprise/cmd/worker/internal/permissions/perms_syncer_scheduler_test.go
@unknwon enterprise/internal/database/perms_store.go

@pjlast pjlast requested a review from a team June 27, 2023 12:14

@pjlast pjlast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🕺

Comment thread enterprise/cmd/worker/internal/permissions/perms_syncer_scheduler_test.go Outdated
@kopancek kopancek merged commit f3fd139 into main Jun 27, 2023
@kopancek kopancek deleted the milan/fix_scheduled_perms_sync_queues branch June 27, 2023 12:27
github-actions Bot pushed a commit that referenced this pull request Jun 27, 2023
…54278)

Previously, the `usersWithOldestPermsQuery` and
`reposWithOldestPermsQuery` relied solely on the permission_sync_jobs
table. If there was no row for the user or repo, this query would not
return that user anymore.

This resulted in users/repos never being scheduled for a permission sync
in cases when a job was not created previously (or jobs table was
truncated).

Attempted to fix the queries by including those users/repos that do not
have any record in the jobs table and putting these users/repos at the
top of the queue.

## Test plan

unit test modified, tested locally

(cherry picked from commit f3fd139)
keegancsmith pushed a commit that referenced this pull request Jun 27, 2023
…lready a job (#54281)

Previously, the `usersWithOldestPermsQuery` and
`reposWithOldestPermsQuery` relied solely on the permission_sync_jobs
table. If there was no row for the user or repo, this query would not
return that user anymore.

This resulted in users/repos never being scheduled for a permission sync
in cases when a job was not created previously (or jobs table was
truncated).

Attempted to fix the queries by including those users/repos that do not
have any record in the jobs table and putting these users/repos at the
top of the queue.



## Test plan

unit test modified, tested locally
 <br> Backport f3fd139 from #54278

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants