Skip to content

feat(controller): Add last hit timestamp to memoization caches#5487

Merged
simster7 merged 4 commits into
argoproj:masterfrom
terrytangyuan:cache-hit-timestamp
Mar 26, 2021
Merged

feat(controller): Add last hit timestamp to memoization caches#5487
simster7 merged 4 commits into
argoproj:masterfrom
terrytangyuan:cache-hit-timestamp

Conversation

@terrytangyuan
Copy link
Copy Markdown
Member

@terrytangyuan terrytangyuan commented Mar 22, 2021

This is the first step mentioned in #5406.

Controller to manage GC of configmaps based on this timestamp will be added as a follow-up PR later as it requires additional work. We can start using this timestamp in our cluster and experiment with different options in the meantime (hopefully gather more experience and feedback on GC strategies through the process). Our immediate need is to have a better way to tell whether a cache is still useful.

cc @simster7 Could you take a look when you get a chance?

Signed-off-by: terrytangyuan terrytangyuan@gmail.com

Checklist:

Comment thread workflow/controller/cache/configmap_cache.go Outdated
@codecov

This comment was marked as resolved.

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
@terrytangyuan terrytangyuan changed the title feat(controller): Add last hit timestamp label to memoization caches feat(controller): Add last hit timestamp to memoization caches Mar 23, 2021
@simster7 simster7 self-assigned this Mar 23, 2021
Copy link
Copy Markdown
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

We surely also "hit" an entry when we write to it no?

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
@terrytangyuan
Copy link
Copy Markdown
Member Author

terrytangyuan commented Mar 25, 2021

We surely also "hit" an entry when we write to it no?

I initially wanted to differentiate from it but I just added it anyways since this differentiation would introduce unnecessary logic in downstream apps (for GC mechanism). Please take another look.

@terrytangyuan terrytangyuan requested a review from simster7 March 25, 2021 17:50
Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
NodeID: nodeId,
Outputs: value,
CreationTimestamp: metav1.Time{Time: time.Now()},
CreationTimestamp: metav1.Time{Time: creationTime},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder what the utility of creation timestamp is now that we introduce last hit timestamp... perhaps maxAge should always reference lastHitTimestamp instead, although this would break some user expecations about how maxAge works....

What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is only when it's firstly saved. When the entry is actually hit/loaded every time, the LastHitTimestamp would change. I think maxAge should stick with CreationTimestamp as the expectation for age is after something is initially born/created. Interesting analysis can be done by utilizing both timestamps, e.g. the cache is very old but still being used so maybe it's time to delete/refresh it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting analysis can be done by utilizing both timestamps, e.g. the cache is very old but still being used so maybe it's time to delete/refresh it?

Yeah I agree, this is good potential. Out of scope for this PR though

@simster7 simster7 merged commit 76b6a0e into argoproj:master Mar 26, 2021
@terrytangyuan terrytangyuan deleted the cache-hit-timestamp branch March 26, 2021 19:20
This was referenced Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/gc Garbage collection, such as TTLs, retentionPolicy, delays, and more area/memoization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants