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.

Sign up for GitHub

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

Jump to bottom

improve query performance #17862

Merged
merged 4 commits into from
Oct 12, 2022
Merged

improve query performance #17862

merged 4 commits into from
Oct 12, 2022

Conversation

colesnodgrass
Copy link
Member

What

  • improve query performance for our worst performing query
    image
  • remove small, but still existent potential for SQL injection by replacing String.format %s with a ? bind variable

How

  • replace temp-table (created via WITH clause) with simpler query EXTRACT(EPOCH FROM...) query
  • may still need to add indexes to the database

@colesnodgrass colesnodgrass temporarily deployed to more-secrets October 11, 2022 23:15 Inactive
@colesnodgrass colesnodgrass temporarily deployed to more-secrets October 12, 2022 02:26 Inactive
jdpgrailsdev
jdpgrailsdev approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@colesnodgrass colesnodgrass merged commit e0db09b into master Oct 12, 2022
@colesnodgrass colesnodgrass deleted the cole/reporter-query-cleanup branch October 12, 2022 16:10
benmoriceau
benmoriceau approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@benmoriceau benmoriceau left a comment

Choose a reason for hiding this comment

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

LGTM: One comment about the lack of test, if not in place it shouldn't be part of this review

airbyte-metrics/reporter/src/main/java/io/airbyte/metrics/reporter/MetricRepository.java
@@ -159,32 +159,17 @@ SELECT status, extract(epoch from age(updated_at, created_at)) AS sec FROM jobs
}

private long oldestJobAgeSecs(final JobStatus status) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT/ FAR (For another review): This is not covert by a UTest. Should we write a story to cover those queries on a container DB. That would make those changes much safer.

malikdiarra added a commit that referenced this pull request Oct 13, 2022
@malikdiarra
Revert "improve query performance (#17862)"
3f85fc4 
This reverts commit e0db09b.
malikdiarra added a commit that referenced this pull request Oct 13, 2022
@malikdiarra
Revert metrics reporter migration to micronaut (#17927)
473b5db 
* Revert "improve query performance (#17862)"

This reverts commit e0db09b.

* Revert "fix metric reporter not producing metrics (#17863)"

This reverts commit 63e39e6.

* Revert "convert airbyte-metrics-reporter to micronaut (#17365)"

This reverts commit d30de1b.
This was referenced Oct 13, 2022
Closed
Merged
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@colesnodgrass @jhammarstedt
improve query performance (airbytehq#17862)
e607e36 
* replace WITH query with more performant query

* replace multiple ORs with an IN
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@malikdiarra @jhammarstedt
Revert metrics reporter migration to micronaut (airbytehq#17927)
85a08a4 
* Revert "improve query performance (airbytehq#17862)"

This reverts commit e0db09b.

* Revert "fix metric reporter not producing metrics (airbytehq#17863)"

This reverts commit 63e39e6.

* Revert "convert airbyte-metrics-reporter to micronaut (airbytehq#17365)"

This reverts commit d30de1b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdpgrailsdev jdpgrailsdev jdpgrailsdev approved these changes

@benmoriceau benmoriceau benmoriceau approved these changes

Assignees

@jdpgrailsdev jdpgrailsdev

@gosusnp gosusnp

@benmoriceau benmoriceau

@alovew alovew

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@colesnodgrass @jdpgrailsdev @benmoriceau @gosusnp @alovew