-
Notifications
You must be signed in to change notification settings - Fork 807
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
feat(sql): Optimize executions #4804
Conversation
fb4fd18
to
40fd05d
Compare
@Nonnull pipelineConfigIds: List<String>, | ||
@Nonnull criteria: ExecutionCriteria | ||
): List<String> { | ||
return primary.retrieveAndFilterPipelineExecutionIdsForApplication(application, pipelineConfigIds, criteria) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MINOR: BUT ... this ordering DOES matter on the selection criteria and don't think we have a test for this (aka primary MUST come first or it'd select previous data potentially). Just an observation that this is missing a test .. but not sure how "critical" that test really is.
var baseQueryPredicate = field("application").eq(application) | ||
.and(field("config_id").`in`(*pipelineConfigIds.toTypedArray())) | ||
|
||
var table = if (jooq.dialect() == SQLDialect.MYSQL) PIPELINE.tableName.forceIndex("pipeline_application_idx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: DO we really want to force an index, or is another index on the combination needed? OR perhaps merging a couple of these indexes. NOT sure best solution but... we have a number of indexes with some of this data and wondering if we couldn't do this a touch cleaner. I'd EXPECT the optimizer to handle MOST of this without the need to force an index.
...rc/main/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepository.kt
Outdated
Show resolved
Hide resolved
...rc/main/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepository.kt
Show resolved
Hide resolved
orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy
Outdated
Show resolved
Hide resolved
orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy
Show resolved
Hide resolved
orca-web/src/main/java/com/netflix/spinnaker/config/TaskControllerConfigurationProperties.java
Outdated
Show resolved
Hide resolved
The whole idea seems like a good optimisation to me 👍 @dbyron-sf @kirangodishala how long have you been running this in your forks? Also could we fit the same concept of optimisations in |
...rc/main/kotlin/com/netflix/spinnaker/orca/sql/pipeline/persistence/SqlExecutionRepository.kt
Outdated
Show resolved
Hide resolved
orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy
Outdated
Show resolved
Hide resolved
orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy
Outdated
Show resolved
Hide resolved
…perties optimize getPipelinesForApplication() by reducing the number of relevant pipeline config ids to query This is a breaking change: TaskController previously looked at these properties: `tasks.days-of-execution-history` `tasks.number-of-old-pipeline-executions-to-include` and now they live in `tasks.controller`. fix(web): give java code access to groovy to fix below error: > Task :orca-web:compileJava FAILED ../orca/orca-web/src/main/java/com/netflix/spinnaker/config/TaskControllerConfigurationProperties.java:19: error: cannot find symbol import com.netflix.spinnaker.orca.controllers.TaskController; ^ symbol: class TaskController location: package com.netflix.spinnaker.orca.controllers 1 error
…ion() further by processing multiple pipeline config ids at a time and multi-threading each config id batch
refactor function and variable names to make them more descriptive
48975d9
to
ddf724e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There.. are long term things I'd like to evaluate - primarily DB indexes & see if we can move some of this logic out of code or simplify it in some way, but... this is a good start on improving the response times. I worry a touch in a COUPLE spots on how it handles things around in-memory/threads, but for now, approved.
This PR improves performance when using sql as the backend by optimizing the way the results are obtained for the api call
/applications/{application}/pipelines?expand=false&limit=2
frequently initiated by deck.Deck makes that call to Gate. Gate further forwards that call to Orca, specifically to
/v2/applications/{application}/pipelines
endpoint.Orca calls front50 to get the pipeline and strategy config ids and then merges them. Then for each id in the merged list, it queries its own db (SQL) to handle the query parameters, such as limit=2 and filter by statuses. It is observed that these db operations are taking longer times and this PR fixes the slowness by optimizing the queries to the db.
Refactored TaskController to support externalized config properties
Optimize getPipelinesForApplication() by reducing the number of relevant pipeline config ids to query
Further optimize getPipelinesForApplication() by processing multiple pipeline config ids at a time and multi-threading each config id batch
Breaking changes:
These configuration properties
are replaced by the below configuration along with few more newly added ones: