Add support for cache TTL #6451
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code Review Agent Run Status
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6451 +/- ##
==========================================
+ Coverage 58.48% 58.53% +0.04%
==========================================
Files 940 940
Lines 71584 71608 +24
==========================================
+ Hits 41867 41915 +48
+ Misses 26534 26516 -18
+ Partials 3183 3177 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sovietaced
force-pushed
the
issue-6450
branch
2 times, most recently
from
bae020b to
822ed4a
Compare
Code Review Agent Run Status
|
3 similar comments
Code Review Agent Run Status
|
Code Review Agent Run Status
|
Code Review Agent Run Status
|
Code Review Agent Run Status
|
1 similar comment
Code Review Agent Run Status
|
Sovietaced
commented
May 30, 2025
| defer func() { | ||
| if r := recover(); r != nil { | ||
| tx.Rollback() | ||
| err := h.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { |
There was a problem hiding this comment.
Mostly refactoring to use the nicer transaction syntax
Sovietaced
commented
May 30, 2025
|
|
||
| func (p *postgresErrorTransformer) ToDataCatalogError(err error) error { | ||
| var dce catalogErrors.DataCatalogError | ||
| if errors.As(err, &dce) { |
There was a problem hiding this comment.
A little improvement so we can liberally call ToDatacatalogError on things especially in nested situations.
Signed-off-by: Jason Parraga <[email protected]>
|
From reading you description it sees expired at is a better primary key member |
Yeah that would work as well. Just need to widen that composite key with something. |
| artifactModel.ArtifactData = artifactDataModels | ||
|
|
||
| // Reset TTL on the artifact since all the data is fresh. | ||
| if request.GetTtl() != nil { |
There was a problem hiding this comment.
This might be problematic depending on how the object storage engine implements the lifecycle rule for the object. It isn't required that the TTL be reset if a /PUT request is issued for a given key. For example, S3 triggers off of the creationTimestamp.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tracking issue
Closes #3305
Why are the changes needed?
Flyte data may be stored in S3 buckets that have retention policies. If those retention policies delete the underlying data in S3 the entries in Flyte data catalog are stale and point to nonexistent buckets. This results in errors at task runtime in flytekit.
What changes were proposed in this pull request?
This pull request modifies Flyte data catalog such that artifacts in the catalog can expire and no longer be valid. The general approach is to allow for multiple rows with the same artifact key but to only retain one non-expired row ideally. I chose to allow for multiple rows to exist so that cleanup of expired artifacts could be done asynchronously. Also, there are multiple tables that hold references to the artifact table and ensuring these dependents were also expired seemed challenging.
Data Model
expires_at. This dictates when the artifact is no longer valid.created_atsuch that there can be multiple rows with the same artifact key (ideally with one and only one non-expired entry if insertions are done carefully and atomically).artifact_idsuch that multiple tags can exist with the same artifact key but can only point to one artifact.Data Layer
Manager Layer
expires_atvalue for records.Propeller
Updated propeller to plumb the cache TTL from the task metadata to Flyte data catalog API calls.
Future Updates
At a high level I'd like to follow up on this work with a background job of some sort that queries the DB for expired cache entries and then thoroughly cleans them up by deleting the contents if possible and then later removing the rows from Postgres. Similar to what is done in #4655
I realize flytekit changes will need to follow up here as well but I think those will be relatively trivial and this change is entirely backwards compatible.
How was this patch tested?
Unit tests, for now.
Labels
Please add one or more of the following labels to categorize your PR:
This is important to improve the readability of release notes.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This pull request enhances the Flyte data catalog by introducing support for cache TTL, allowing artifacts to expire and preventing stale entries that could lead to runtime errors. Key modifications include the addition of an 'expires_at' column in the artifacts table and updates to the data layer for filtering expired artifacts. The implementation is designed to be backward compatible and includes unit tests to validate the new functionality.