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

repoupdater(scheduler): Prevent race condition of schedule preloading#63086

Merged
eseliger merged 1 commit into
mainfrom
es/06-04-repoupdaterschedulerpreventraceconditionofschedulepreloading
Jun 4, 2024
Merged

repoupdater(scheduler): Prevent race condition of schedule preloading#63086
eseliger merged 1 commit into
mainfrom
es/06-04-repoupdaterschedulerpreventraceconditionofschedulepreloading

Conversation

@eseliger

@eseliger eseliger commented Jun 4, 2024

Copy link
Copy Markdown
Member

My local instance has few repos enough that this doesn't happen, but on larger instances this preloading fights with the new preloading.
They are both best effort, and are meant to achieve the same thing.
Thus, this one is not required anymore, and we can delete it, after we added another one in https://github.com/sourcegraph/sourcegraph/pull/62891.

Test plan:

Verified with sleeps and logs locally that repos are correctly upserted in the schedule now.

@cla-bot cla-bot Bot added the cla-signed label Jun 4, 2024

eseliger commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @eseliger and the rest of your teammates on Graphite Graphite

@github-actions github-actions Bot added team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all labels Jun 4, 2024
@eseliger eseliger marked this pull request as ready for review June 4, 2024 20:27
@eseliger eseliger requested a review from a team June 4, 2024 20:27
Comment thread internal/repos/syncer.go Outdated

@mmanela mmanela Jun 4, 2024

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.

Were there no tests for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sigh - no. The scheduler and all the ~10 things that work together to fill the schedule are very poorly tested unfortunately :/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

actually - I take that back, this was covered by another test :)

My local instance has few repos enough that this doesn't happen, but on larger instances this preloading fights with the new preloading.
They are both best effort, and are meant to achieve the same thing.
Thus, this one is not required anymore, and we can delete it, after we added another one in https://github.com/sourcegraph/sourcegraph/pull/62891.

Test plan:

Verified with sleeps and logs locally that repos are correctly upserted in the schedule now.
@eseliger eseliger force-pushed the es/06-04-repoupdaterschedulerpreventraceconditionofschedulepreloading branch from 08f9403 to 3407fdc Compare June 4, 2024 20:47
@eseliger eseliger merged commit 252abb5 into main Jun 4, 2024
@eseliger eseliger deleted the es/06-04-repoupdaterschedulerpreventraceconditionofschedulepreloading branch June 4, 2024 20:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/source Tickets under the purview of Source - the one Source to graph it all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants