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

Mitigation for skipped pipelines (issue #316) #317

Merged
merged 1 commit into from
Dec 6, 2022

Conversation

zackgalbreath
Copy link
Collaborator

@zackgalbreath zackgalbreath commented Sep 6, 2022

See #316 for a description of the problem that this is mitigating

@zackgalbreath zackgalbreath marked this pull request as draft September 6, 2022 18:58
@zackgalbreath
Copy link
Collaborator Author

This approach seems to work, but it's pretty slow (~9 - 10 minutes per run).

@kotfic has some prior art that directly queries the GitLab database instead of repeatedly querying the API.

@mvandenburgh do you think that approach is still sound after our move to AWS RDS? I'm not sure how to connect to that database to test / debug a script like this.

@mvandenburgh
Copy link
Member

@mvandenburgh do you think that approach is still sound after our move to AWS RDS? I'm not sure how to connect to that database to test / debug a script like this.

Yeah, anything like that that worked before with the in-cluster DB should still work. The RDS database lives in the same VPC as the old one, so from the perspective of the components of the cluster that directly interact with the DB, nothing has really changed, it's still just a postgres database.

@zackgalbreath zackgalbreath marked this pull request as ready for review October 20, 2022 20:27
@zackgalbreath
Copy link
Collaborator Author

@mvandenburgh what do you think about merging this mitigation as-is and trying to optimize its runtime later if it actually proves to be a problem?

mvandenburgh
mvandenburgh previously approved these changes Oct 21, 2022
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

Sounds good to me. Since this solution is proven to work, I agree we should merge as-is and focus on higher priority things (as long as peformance is sufficient)

@zackgalbreath zackgalbreath merged commit 3348a08 into main Dec 6, 2022
@zackgalbreath zackgalbreath deleted the skipped_pipelines branch December 6, 2022 15:23
@tgamblin
Copy link
Member

tgamblin commented Dec 6, 2022

Can this description be updated with rationale?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants