Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add count to execution repo interfaces #464

Merged
merged 5 commits into from
Aug 26, 2022

Conversation

andrewwdye
Copy link
Contributor

TL;DR

Add a Count interface and implementation to ExecutionRepo, NodeExecutionRepo, and TaskExecutionRepo. This allows counting executions matching a filter set without needing to materialize the list.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Workflow, node, and task execution states are stored in a GORM DB. This change adds a Count interface to ExecutionRepo, NodeExecutionRepo, and TaskExecutionRepo to query the count of executions efficiently (instead of materializing the list and summing). The interface accepts InlineFilters and MapFilters, similar to the List interfaces and the ListResourceInput param.

Relevant unit tests added.

This change also

  • Runs make goimports, which led to updates in a number of unrelated files
  • Refactors existing repo mocks for consistency

Tracking Issue

NA

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Aug 24, 2022

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #464 (e1c330d) into master (b19ab2a) will increase coverage by 0.07%.
The diff coverage is 74.57%.

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   61.51%   61.59%   +0.07%     
==========================================
  Files         157      157              
  Lines       11248    11305      +57     
==========================================
+ Hits         6919     6963      +44     
- Misses       3612     3619       +7     
- Partials      717      723       +6     
Flag Coverage Δ
unittests 60.53% <73.21%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
auth/init_secrets.go 0.00% <ø> (ø)
pkg/errors/errors.go 54.00% <ø> (ø)
pkg/manager/impl/executions/quality_of_service.go 64.34% <ø> (ø)
pkg/repositories/errors/postgres.go 80.48% <ø> (ø)
pkg/repositories/gormimpl/resource_repo.go 66.03% <ø> (ø)
pkg/repositories/gormimpl/execution_repo.go 71.11% <68.18%> (-0.95%) ⬇️
pkg/repositories/gormimpl/node_execution_repo.go 68.24% <73.33%> (+0.57%) ⬆️
pkg/repositories/gormimpl/task_execution_repo.go 64.35% <75.00%> (+2.00%) ⬆️
pkg/repositories/gormimpl/metrics.go 100.00% <100.00%> (ø)
pkg/repositories/gormimpl/common.go 67.85% <0.00%> (+7.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andrewwdye andrewwdye force-pushed the execution-repo-count branch from f2b0ec8 to bb956c9 Compare August 25, 2022 15:46
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

looks great! thanks for adding all the clean-up too

pkg/manager/impl/execution_manager_test.go Show resolved Hide resolved
pkg/repositories/gormimpl/execution_repo.go Show resolved Hide resolved
pkg/server/service.go Show resolved Hide resolved
@andrewwdye
Copy link
Contributor Author

Fyi, created flyteorg/flyte#2814 for previously failing Check Go Generate. Looks like an inconsistency between go 1.18 and 1.19.

@katrogan katrogan merged commit cdc5c3d into flyteorg:master Aug 26, 2022
@welcome
Copy link

welcome bot commented Aug 26, 2022

Congrats on merging your first pull request! 🎉

eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants