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

feat(cherry-picks): Adding selected improvements to 1.36.x release #4830

Closed

Conversation

christosarvanitis
Copy link
Member

Cherry-picked the following commits:

93f27cca937ff4b4411ce046ee662dba615a47d1 #4824
386da272330a4c1d1fd44b284099dd01f2794e7a #4803
d739295d626ce7a5600537107565d32d583cd741 #4804

kirangodishala and others added 3 commits February 5, 2025 11:26
* feat(web): refactor TaskController to support externalized config properties

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

* feat(taskController): optimize TaskController.getPipelinesForApplication() further by processing multiple pipeline config ids at a time and multi-threading each config id batch

* refactor(taskController): rework execution retrieval query optimizations

* feat(taskController): have a dedicated executor service threadpool per request

* fix(sql): add correct indexes

refactor function and variable names to make them more descriptive

* feat(taskController): add query timeouts when retrieving execution body

* fix(log): minor log refactor

* fix(taskController): adjusted default config values

---------

Co-authored-by: Apoorv Mahajan <[email protected]>
…s in SqlExecutionRepository (spinnaker#4803)

* test(sql): demonstrate that SqlConfiguration makes different types of DataSource bean available
depending on configuration

* feat(sql): use a connection pool named "read" for read operations in SqlExecutionRepository if configured to do so.

WIP: so far this is only configuration, nothing actually uses the read pool

* refactor(sql): reorganize correlation id cleanup logic in SqlExecutionRepository

to pave the way to use the read pool for selecting by correlation id

* perf(sql): no need for a transaction to delete individual rows

- correlation ids for completed pipelines/orchestrations
- a stage

* perf(sql): use the read pool for read-only database queries in SqlExecutionRepository

* refactor(sql): remove forUpdate argument from SqlExecutionRepository.selectExecution

since it's not used.

---------

Co-authored-by: David Byron <[email protected]>
…innaker#4824)

* test(pipeline): Define StartStageHandler performance

when given a complex pipeline with multiple layers of upstream stages

* fix(pipeline): Memoize anyUpstreamStagesFailed results

This turns the anyUpstreamStagesFailed calculation from one that
scales exponentially based on the number of (branches+downstream stages)
in a pipeline to one that scales linearly based on the total number
of stages in a pipeline. This is a significant performance improvement,
especially for very large and complicated pipelines

* refactor(stage): Move anyUpstreamStagesFailed to StartStageHandler

since that's the only place where it gets used

* fix(stage): Avoid using a ConcurrentHashMap

for memoization. The recursive anyUpstreamStagesFailed(StageExecution)
function runs in a single thread, so ConcurrentHashMap is not
necessary here

* docs(test): Use a more concise test name

* perf(stage): First check if a stage has been visited

before checking for parent stages. stage.getRequisiteStageRefIds
is a more expensive call because the underlying implementation creates
a copy of a List. Therefore, start with the cheaper operation first
hoping to short-circuit and avoid the more expensive check

* perf(stage): Filter out non-synthetic stages

The javadocs state that the syntheticStageOwner property is null
for non-synthetic stages. Use this information to filter out non-synthetic
stages before attempting a potentially expensive operation
to check for synthetic parents of previousStages

* perf(stage): Precompute requisiteStageRefIds

StageExecutionImpl.getRequisiteStageRefIds() returns a copy of a
Set. This is a costly operation that has the potential to get repeated
for every unvisited stage. To avoid this, compute the value before
entering a loop

* perf(stage): Only use withAuth when needed

withAuth is only necessary when starting a stage. Since withAuth is
very computationally expensive for complex pipelines, only call it
when it is absolutely necessary.

* perf(stage): Remove duplicate call to withStage

StartStageHandler already makes a call to message.withStage at the
beginning of the handle() method. Therefore, this call within the
catch block is unnecessary

* chore(import): Clean up unused imports

---------

Co-authored-by: Daniel Zheng <[email protected]>
@dbyron-sf
Copy link
Contributor

Could you use mergify or at least do these in separate PRs please?

@christosarvanitis
Copy link
Member Author

Could you use mergify or at least do these in separate PRs please?

This was never intended to be raised as a PR here. Messed up the PR on a different fork. Please ignore

@spinnakerbot
Copy link
Contributor

Features cannot be merged into release branches. The following commits are not tagged as one of "fix, chore, docs, perf, test":

Read more about commit conventions and patch releases here.

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.

5 participants