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

perf(artifacts): use pageSize=1 when resolving prior artifacts (#2955) #2990

Merged

Conversation

mattnworb
Copy link
Contributor

This is a smaller-scoped attempt at
#2938, which relates to
spinnaker/spinnaker#4367.

ArtifactResolver.resolveArtifacts(Map) currently attempts to load
all executions for a pipeline in order to find the most recent
execution in the course of resolving expected artifacts. This causes a
lot of unnecessary data to be loaded from Redis/SQL, only to be
discarded a few operations later.

This change makes use of the fact that the ExecutionRepository
implementations will respect the executionCriteria.pageSize argument
when retrieving Execitions for a pipelineId.

In the Redis-based implementation, the executions are stored in a sorted
set scored on buildTime (or currentTimeMillis() if buildTime is
null), so retrieving all of the executions for the pipelineId with a
pageSize=1 should load the Execution with the most recent buildTime.

In the SQL-based implementation,
retrievePipelinesForPipelineConfigId(String, ExecutionCriteria) sorts
the query results based on the id field.

For both implementations, this is a small change from the existing
behavior of ArtifactResolver, which retrieves all executions and then
uses the one with the most recent startTime (or id). This change
seems like it will lead to the same result in most cases though, since
buildTime is set when the Execution is stored, and the id field is
ascending based on the timestamp of when it is generated.

The retrievePipelinesForPipelienConfigId method in both
implementations currently ignores the executionCriteria.sortType
field, but I've added this in the call from ArtifactResolver to at
least document ArtifactResolver's desire.

…aker#2955)

This is a smaller-scoped attempt at
spinnaker#2938, which relates to
spinnaker/spinnaker#4367.

`ArtifactResolver.resolveArtifacts(Map)` currently attempts to load
_all_ executions for a pipeline in order to find the most recent
execution in the course of resolving expected artifacts. This causes a
lot of unnecessary data to be loaded from Redis/SQL, only to be
discarded a few operations later.

This change makes use of the fact that the ExecutionRepository
implementations will respect the `executionCriteria.pageSize` argument
when retrieving Execitions for a pipelineId.

In the Redis-based implementation, the executions are stored in a sorted
set scored on `buildTime` (or `currentTimeMillis()` if `buildTime` is
null), so retrieving all of the executions for the pipelineId with a
pageSize=1 should load the Execution with the most recent `buildTime`.

In the SQL-based implementation,
`retrievePipelinesForPipelineConfigId(String, ExecutionCriteria)` sorts
the query results based on the `id` field.

For both implementations, this is a small change from the existing
behavior of ArtifactResolver, which retrieves all executions and then
uses the one with the most recent `startTime` (or `id`). This change
seems like it will lead to the same result in most cases though, since
buildTime is set when the Execution is stored, and the `id` field is
ascending based on the timestamp of when it is generated.

The `retrievePipelinesForPipelienConfigId` method in both
implementations currently ignores the `executionCriteria.sortType`
field, but I've added this in the call from ArtifactResolver to at
least document ArtifactResolver's desire.
@mattnworb
Copy link
Contributor Author

and here is another cherry-pick of #2955 against 1.13.x @maggieneterval @ezimanyi

@ezimanyi ezimanyi merged commit 3072865 into spinnaker:release-1.13.x Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants