feat(backend) V1 Pipelines add maximum_cache_staleness and default_cache_staleness #8270
Conversation
|
@juliusvonkohout: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@chensun @james-jwu @zijianjoy as discussed yesterday in the KFP meeting i split off the simple cache changes. Please review and merge. |
|
Thank you @juliusvonkohout! The PR looks good to me, just requesting some small changes. |
@Linchin is there something else that must be done? I think my new tests are sufficient for the basic functionality. |
|
@juliusvonkohout Thank you for the work! Looks good. |
Set default to infinity. stalenessToSeconds() returns -1 on empty strings.
juliusvonkohout
requested review from
chensun
and removed request for
gkcalat and
zijianjoy
juliusvonkohout
requested review from
Linchin and
chensun
and removed request for
chensun and
Linchin
| userCacheStalenessInSeconds = stalenessToSeconds(userCacheStaleness) | ||
| log.Printf("userCacheStalenessInSeconds: %d", userCacheStalenessInSeconds) | ||
| } | ||
| defaultCacheStaleness, exists := os.LookupEnv("DEFAULT_CACHE_STALENESS") |
There was a problem hiding this comment.
If userCacheStaleness exists, we shouldn't even care about defaultCacheStaleness, right?
There was a problem hiding this comment.
Yes, exactly that is happening. I just read the two environment variables and the Workflow annotation.
Then i calculate from all three values the staleness to use. So if a user/annotation value is set and less than MAXIMUM_CACHE_STALENESS, the user/annotation value is used instead of DEFAULT_CACHE_STALENESS.
var cacheStalenessInSeconds int64 = -1
var userCacheStalenessInSeconds int64 = -1
var defaultCacheStalenessInSeconds int64 = -1
var maximumCacheStalenessInSeconds int64 = -1
userCacheStaleness, exists := annotations[MaxCacheStalenessKey]
if exists {
userCacheStalenessInSeconds = stalenessToSeconds(userCacheStaleness)
}
defaultCacheStaleness, exists := os.LookupEnv("DEFAULT_CACHE_STALENESS")
if exists {
defaultCacheStalenessInSeconds = stalenessToSeconds(defaultCacheStaleness)
}
maximumCacheStaleness, exists := os.LookupEnv("MAXIMUM_CACHE_STALENESS")
if exists {
maximumCacheStalenessInSeconds = stalenessToSeconds(maximumCacheStaleness)
}
if userCacheStalenessInSeconds < 0 {
cacheStalenessInSeconds = defaultCacheStalenessInSeconds
} else {
cacheStalenessInSeconds = userCacheStalenessInSeconds
}
if cacheStalenessInSeconds > maximumCacheStalenessInSeconds {
cacheStalenessInSeconds = maximumCacheStalenessInSeconds
}
log.Printf("cacheStalenessInSeconds: %d", cacheStalenessInSeconds)
There was a problem hiding this comment.
Yes. I was thinking that something like this would be cleaner and free from waste operation. I don't have strong opinion though.
var cacheStalenessInSeconds int64 = -1
cacheStaleness, exists := annotations[MaxCacheStalenessKey]
if !exists {
cacheStaleness, exists = os.LookupEnv("DEFAULT_CACHE_STALENESS")
}
if exists {
cacheStalenessInSeconds = stalenessToSeconds(cacheStaleness)
}
...
if cacheStalenessInSeconds > maximumCacheStalenessInSeconds {
cacheStalenessInSeconds = maximumCacheStalenessInSeconds
}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
"2022-10-04 17:05:01 ERROR 503: Egress is over the account limit.." the build systems seems to be overloaded. i will rerun the tests. |
|
/retest-required |
juliusvonkohout
changed the title
…kubeflow#8270) * Update pipeline-install-config.yaml * Update cache-deployment.yaml * Update execution_cache_store_test.go * Update execution_cache_store.go * Update watcher.go * Update mutation.go * fix identation * Update execution_cache_store_test.go * Update execution_cache_store.go * Update pipeline-install-config.yaml Set default to infinity. stalenessToSeconds() returns -1 on empty strings. * remove logging as requested by chensun
Description of your changes:
Part of #8177
Checklist: