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

Fetch invocation history from clickhouse, merge in unfinished builds from mysql #6447

Merged
merged 5 commits into from
Apr 30, 2024

Conversation

jdhollen
Copy link
Member

Related issues: N/A

@jdhollen jdhollen changed the title Functioning, no tests Fetch invocation history from clickhouse, merge in unfinished builds from mysql (WIP) Apr 25, 2024
@jdhollen jdhollen marked this pull request as ready for review April 25, 2024 19:50
@jdhollen
Copy link
Member Author

Fully intending to write tests, but hoping to have you all take a look and decide if this is worth it or not.

This only improves performance for queries where the user is either:

  1. Not really filtering a ton + using the default sort order (because the range we query from mysql is small), or
  2. Filtering out pending builds (because we won't go to mysql at all)

Short of doing something that lets us skip MySQL entirely, you need some version of this hack.

If we don't add an index for overall status, then queries for other sort orders will stay slow--they'll require a full scan of all 30 days or whatever the user is looking at (i don't think it's reasonable to add an index for every sort order).

If we add an index and it's still not enough for some orgs, we could make performance better by marking long-disconnected and long-pending builds as "completed" with a janitor task (and flush to clickhouse)--then we could rely the status index to make the MySQL queries for "actually-pending" builds fast, since there shouldn't be that many per org at any point in time.

Comment on lines 40 to 41
blendedInvocationSearchEnabled = flag.Bool("app.blended_invocation_search_enabled", false, "If true, InvocationSearchService will query clickhouse *and* the regular db.")
olapInvocationSearchEnabled = flag.Bool("app.olap_invocation_search_enabled", true, "If true, InvocationSearchService will query clickhouse for some queries.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: I couldn't figure out the difference between these 2 flags by reading the help strings, maybe the second flag could use more detail (or just have the 1 "blended" flag?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Did what I can here to make this clearer (if wordier)

Copy link
Member Author

@jdhollen jdhollen left a comment

Choose a reason for hiding this comment

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

Alright, gonna add tests and submit.

Comment on lines 40 to 41
blendedInvocationSearchEnabled = flag.Bool("app.blended_invocation_search_enabled", false, "If true, InvocationSearchService will query clickhouse *and* the regular db.")
olapInvocationSearchEnabled = flag.Bool("app.olap_invocation_search_enabled", true, "If true, InvocationSearchService will query clickhouse for some queries.")
Copy link
Member Author

Choose a reason for hiding this comment

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

Did what I can here to make this clearer (if wordier)

@jdhollen jdhollen changed the title Fetch invocation history from clickhouse, merge in unfinished builds from mysql (WIP) Fetch invocation history from clickhouse, merge in unfinished builds from mysql Apr 30, 2024
@jdhollen jdhollen merged commit 4e8e6bc into master Apr 30, 2024
19 checks passed
@jdhollen jdhollen deleted the jh/2024-04-24_blended branch April 30, 2024 21:50
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.

None yet

3 participants