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

Cache Serailize API #47

Merged
merged 60 commits into from
Nov 24, 2021
Merged

Cache Serailize API #47

merged 60 commits into from
Nov 24, 2021

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Aug 24, 2021 •
edited
Loading

TL;DR

Adding artifact reservation API. This construct will be used to restrict concurrent execution of multiple instances of a cached task with the same inputs, mitigating the overhead of identical workloads.

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

Implementing GetOrReserveArtifact and RelaseReservation API calls to manage artifact reservations. The GetOrReserveArtifact call returns the cached artifact if it exists, if not it performs a conditional upsert to claim the artifact reservation. If a task currently holds an artifact reservation, this call is meant to be repeated to update the reservation expiration until completion. Once completed, the task creates artifact using CreateArtifact and releases the reservation with ReleaseReservation.

Waiting on flyteorg/flyteidl#215 to remove ExtendReservation API call and include expiration time and recommended heartbeat interval in GetOrReserveArtifact response.

Tracking Issue

flyteorg/flyte#267
flyteorg/flyte#420
flyteorg/flyte#872

Follow-up issue

NA

@welcome
Copy link

welcome bot commented Aug 24, 2021

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, 2021 •
edited
Loading

Codecov Report

Merging #47 (df8dba4) into master (be4b23e) will increase coverage by 2.48%.
The diff coverage is 81.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   67.21%   69.69%   +2.48%     
==========================================
  Files          28       31       +3     
  Lines        1031     1231     +200     
==========================================
+ Hits          693      858     +165     
- Misses        279      304      +25     
- Partials       59       69      +10
Flag Coverage Δ
unittests 68.72% <80.62%> (+2.67%) ⬆️

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

Impacted Files Coverage Δ
pkg/repositories/factory.go 0.00% <ø> (ø)
pkg/repositories/handle.go 25.53% <0.00%> (-1.14%) ⬇️
pkg/repositories/postgres_repo.go 0.00% <0.00%> (ø)
pkg/manager/impl/reservation_manager.go 81.30% <81.30%> (ø)
pkg/repositories/gormimpl/reservation.go 85.96% <85.96%> (ø)
pkg/repositories/transformers/reservation.go 92.59% <92.59%> (ø)
pkg/repositories/gormimpl/metrics.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be4b23e...df8dba4. Read the comment docs.

@hamersaw hamersaw force-pushed the feature/artifact-reservation branch from 8e3778c to 1627131 Compare August 26, 2021 04:22
@hamersaw hamersaw marked this pull request as draft September 7, 2021 14:45
@hamersaw hamersaw marked this pull request as ready for review September 28, 2021 12:58
@hamersaw hamersaw changed the title [WIP] Implement Artifact Reservation API Cache Serailize API Sep 28, 2021
@hamersaw hamersaw requested a review from EngHabu September 28, 2021 12:58
@hamersaw hamersaw force-pushed the feature/artifact-reservation branch from 72e0494 to e1d6847 Compare September 28, 2021 13:19
milton0825 and others added 20 commits September 28, 2021 08:21
@milton0825 @hamersaw
WIP: add reservation apis
27ca6d6 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add missing files
03fce3d 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
added create DAO
bce5b9c 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
Add get dao
ca1b6dd 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
wip
20d1568 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
fix tests
e80137e 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
wired reservation manager
0211470 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add todos
3c16c66 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add more tests
9783b00 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add more tests
0b6135a 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add more logging
4457b5b 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add logging and stats
60b2c8d 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
fix lint
acccd0a 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
add more comments
e2c6b40 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
First -> Take
bf3011e 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
WIP: add createOrupdate API
eb5a4e7 
Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@yindia @hamersaw
Added boilerplate automation (flyteorg#41)
f979633 
Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@samhita-alla @hamersaw
update boilerplate code (flyteorg#40)
5d93425 
Signed-off-by: Samhita Alla <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@milton0825 @hamersaw
Upgrade gorm to v1.21.9 (flyteorg#42)
7892458 
Signed-off-by: Daniel Rammer <[email protected]>
@yindia @hamersaw
Added boilerplate automation (flyteorg#43)
c0fbbca 
Signed-off-by: Yuvraj <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
hamersaw and others added 6 commits September 28, 2021 08:21
@hamersaw
updated reservation API to only work with reservations - not actual a…
e65570a 
…rtifacts.

Signed-off-by: Daniel Rammer <[email protected]>
@EngHabu @hamersaw
Fix error type check to detect uniqueConstraintViolation
06fcf90 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@EngHabu @hamersaw
Revert "Fix error type check to detect uniqueConstraintViolation"
c21349c 
This reverts commit 2ba0775.

Signed-off-by: Daniel Rammer <[email protected]>
@EngHabu @hamersaw
Fix gorm wrong error type cast (flyteorg#48)
2c968ee 
* Fix gorm wrong error type cast

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Fix unit tests

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Support unwrapping errors to detect connection problems

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* cleanup

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
added support for heartbeat_interval definition in reservation manager
bb05d0f 
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
updated test and fixed lint errors
5b46ec6 
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw hamersaw force-pushed the feature/artifact-reservation branch from e1d6847 to 5b46ec6 Compare September 28, 2021 13:22
EngHabu
EngHabu reviewed Nov 14, 2021
View reviewed changes
boilerplate/flyte/golang_dockerfile/Dockerfile.GoTemplate Outdated Show resolved Hide resolved
pkg/manager/impl/reservation_manager.go Show resolved Hide resolved
pkg/manager/impl/reservation_manager.go Outdated Show resolved Hide resolved
pkg/runtime/configs/data_catalog_config.go Outdated Show resolved Hide resolved
@hamersaw
Merge branch 'master' into feature/artifact-reservation
36e723c
@hamersaw
udpated dockerfile go template to reflect current master fixing rebas…
2faa715 
…e overwrites

Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
and again .. with a space
0a956dc 
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
add docs on exports
c61fef9 
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
changed configuration to use config.Duration from flytestdlib instead…
efab0f2 
… of defining on seconds

Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
added owner id to reservation gorm impl delete function
2ee1f41 
Signed-off-by: Daniel Rammer <[email protected]>
@hamersaw
if reservation is missing on release reservation (meaning another ent…
f52a8cf 
…ity acquired it) then gracefully fail

Signed-off-by: Daniel Rammer <[email protected]>
EngHabu
EngHabu previously approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

👍🏽

EngHabu
EngHabu reviewed Nov 17, 2021
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@hamersaw hamersaw requested a review from EngHabu November 24, 2021 15:57
@EngHabu EngHabu merged commit b1b4f65 into flyteorg:master Nov 24, 2021
@welcome
Copy link

welcome bot commented Nov 24, 2021

Congrats on merging your first pull request! 🎉

@hamersaw hamersaw deleted the feature/artifact-reservation branch December 1, 2021 18:40
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
@hamersaw @milton0825
@yindia @samhita-alla @EngHabu
Cache Serailize API (#47)
a3a6c7f 
* WIP: add reservation apis

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add missing files

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* added create DAO

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* Add get dao

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* wip

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* fix tests

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* wired reservation manager

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add todos

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add more tests

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add more tests

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add more logging

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add logging and stats

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* fix lint

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add more comments

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* First -> Take

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* WIP: add createOrupdate API

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* Added boilerplate automation (#41)

Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* update boilerplate code (#40)

Signed-off-by: Samhita Alla <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* Upgrade gorm to v1.21.9 (#42)

Signed-off-by: Daniel Rammer <[email protected]>

* Added boilerplate automation (#43)

Signed-off-by: Yuvraj <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add more instructinos

Use upsert

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add more comments

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* refactor a bit

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add timer

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* fix lint / tests

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add comments & tests

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* fix lint

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* add docs

Signed-off-by: Chao-Han Tsai <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* Fix connection error handling (#45)

Signed-off-by: Daniel Rammer <[email protected]>

* Update code of conduct (#46)

Signed-off-by: Ketan Umare <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* separated ReservationManager CreateOrUpdate function into individual Create and Update functions

Signed-off-by: Daniel Rammer <[email protected]>

* fixed race condition on Reservation repository Create function

Signed-off-by: Daniel Rammer <[email protected]>

* changed reservation expiration to use heartbeatInterval and heartbeatGracePeriodMultiplier

Signed-off-by: Daniel Rammer <[email protected]>

* fixed lint and unit test errors

Signed-off-by: Daniel Rammer <[email protected]>

* added unit tests for extending reservation and update failure

Signed-off-by: Daniel Rammer <[email protected]>

* removed ExtendReservation API mocks

Signed-off-by: Daniel Rammer <[email protected]>

* added ExpiresAt and HeartbeatInterval fiedls to ReservationStatus return based on new API

Signed-off-by: Daniel Rammer <[email protected]>

* implemented ReleaseReservation

Signed-off-by: Daniel Rammer <[email protected]>

* added unit tests for reservation transformer

Signed-off-by: Daniel Rammer <[email protected]>

* fixed lint errors

Signed-off-by: Daniel Rammer <[email protected]>

* implemented unit tests for ReleaseReservation API call

Signed-off-by: Daniel Rammer <[email protected]>

* updated reservation API to only work with reservations - not actual artifacts.

Signed-off-by: Daniel Rammer <[email protected]>

* Fix error type check to detect uniqueConstraintViolation

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* Revert "Fix error type check to detect uniqueConstraintViolation"

This reverts commit 22c2d03.

Signed-off-by: Daniel Rammer <[email protected]>

* Fix gorm wrong error type cast (#48)

* Fix gorm wrong error type cast

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Fix unit tests

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Support unwrapping errors to detect connection problems

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* cleanup

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Daniel Rammer <[email protected]>

* added support for heartbeat_interval definition in reservation manager

Signed-off-by: Daniel Rammer <[email protected]>

* updated test and fixed lint errors

Signed-off-by: Daniel Rammer <[email protected]>

* removed unnecessary dependencies from go.mod

Signed-off-by: Daniel Rammer <[email protected]>

* updated flyteidl version - change before merging

Signed-off-by: Daniel Rammer <[email protected]>

* adding reservation model to migration

Signed-off-by: Daniel Rammer <[email protected]>

* udpated dockerfile go template to reflect current master fixing rebase overwrites

Signed-off-by: Daniel Rammer <[email protected]>

* and again .. with a space

Signed-off-by: Daniel Rammer <[email protected]>

* add docs on exports

Signed-off-by: Daniel Rammer <[email protected]>

* changed configuration to use config.Duration from flytestdlib instead of defining on seconds

Signed-off-by: Daniel Rammer <[email protected]>

* added owner id to reservation gorm impl delete function

Signed-off-by: Daniel Rammer <[email protected]>

* if reservation is missing on release reservation (meaning another entity acquired it) then gracefully fail

Signed-off-by: Daniel Rammer <[email protected]>

* updated flyteidl version

Signed-off-by: Daniel Rammer <[email protected]>

* remove flyteidl replace in go.mod and updating to latest version

Signed-off-by: Daniel Rammer <[email protected]>

Co-authored-by: Chao-Han Tsai <[email protected]>
Co-authored-by: Yuvraj <[email protected]>
Co-authored-by: Samhita Alla <[email protected]>
Co-authored-by: Haytham Abuelfutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EngHabu EngHabu EngHabu approved these changes

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hamersaw @EngHabu @milton0825 @yindia @samhita-alla