-
Notifications
You must be signed in to change notification settings - Fork 130
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
DEVPROD-12696: Parallelize waterfall queries #8672
DEVPROD-12696: Parallelize waterfall queries #8672
Conversation
45bce22
to
eb1425a
Compare
graphql/version_resolver.go
Outdated
// If activated is nil in the db we should resolve it and cache it for subsequent queries. There is a very low likely hood of this field being hit | ||
if obj.Activated == nil { | ||
version, err := model.VersionFindOne(model.VersionById(versionId)) | ||
if err != nil { | ||
return nil, InternalServerError.Send(ctx, fmt.Sprintf("fetching version '%s': %s", versionId, err.Error())) | ||
} | ||
if err = setVersionActivationStatus(ctx, version); err != nil { | ||
return nil, InternalServerError.Send(ctx, fmt.Sprintf("setting version activation status: %s", err.Error())) | ||
} | ||
obj.Activated = version.Activated | ||
} |
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.
Is this still necessary? We introduced the activated field a few years ago and have since introduced the TTL I don't think any versions that haven't been TTL'd should still have this field as nil.
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.
Thanks for the context, this was copy pasta. Removed!
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.
I think this doesn't require an app reviewer; gave a quick skim and it overall looks good!
) | ||
|
||
const ( | ||
buildsKey = "builds" |
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.
nit: since this is only used as a private const in getVersionTasksPipeline, we can just define it as a const in there
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.
It's also used by GetWaterfallBuildVariants
commit 4e3ed98 Author: Zackary Santana <[email protected]> Date: Thu Jan 30 13:34:26 2025 -0500 Revert "DEVPROD-13976 Add otel to presign file func (evergreen-ci#8673)" This reverts commit c856008. commit c856008 Author: Zackary Santana <[email protected]> Date: Thu Jan 30 09:30:17 2025 -0500 DEVPROD-13976 Add otel to presign file func (evergreen-ci#8673) commit 6d420a2 Author: Sophie Stadler <[email protected]> Date: Wed Jan 29 14:35:01 2025 -0500 DEVPROD-12696: Parallelize waterfall queries (evergreen-ci#8672)
DEVPROD-12696
Description
The waterfall schema currently returns versions separately from the page's build variants. This was a helpful schema for rendering the page easily, but limited how much we could leverage parallelization and Apollo caching.
I've added a field within the
Version
type that returns all of the builds and tasks for that version. This means that instead of one large query, there are several that run in parallel. The new pipeline reuses a large chunk of the old one so it's pretty well tested. Though organizing the builds into build variants now needs to be done on the client side, performance overall should be faster.Testing
Added unit tests
On Honeycomb, you can see that
Version/waterfallBuilds
runs a lot faster than the existingVersion/buildVariants
query. I'm excited to see how this scales on prod data vs the current aggregation.