Add ability for aggro cache cleanup to only consider Kraken's disk usage #386
Conversation
| testStats := tally.NewTestScope("", map[string]string{}) | ||
| m.clean(testStats, config, op) | ||
| snapshot := testStats.Snapshot() | ||
| usageGauge, ok := snapshot.Gauges()["disk_usage+"] |
There was a problem hiding this comment.
Not sure where the + comes from. Could this be a bug?
There was a problem hiding this comment.
no it's just how tally does the metrics names, in the monorepo we have a tallytest library that takes care of these quirks
Anton-Kalpakchiev
force-pushed
the
aggro-cleanup-two
branch
from
c332836 to
af94237
Compare
Anton-Kalpakchiev
force-pushed
the
aggro-cleanup-two
branch
from
af94237 to
59471a7
Compare
|
|
||
| // calculateDiskUtil calculates the disk space utilization based on the config. | ||
| // If 'ExcludeOtherServices' is turned on, only this op's utilization of the filesystem is considered. | ||
| func calculateDiskUtil(op base.FileOp, config CleanupConfig) (int, error) { |
There was a problem hiding this comment.
How could I test this?
There was a problem hiding this comment.
no way around dependency injection
| Interval time.Duration `yaml:"interval"` // How often cleanup runs. | ||
| TTI time.Duration `yaml:"tti"` // Time to idle based on last access time. | ||
| TTL time.Duration `yaml:"ttl"` // Time to live regardless of access. If 0, disables TTL. | ||
| ExcludeOtherServices bool `yaml:"exclude_other_services"` // Whether to exclude other services from the disk util calculation. |
There was a problem hiding this comment.
@Anton-Kalpakchiev why are we making this config-driven?
In my opinion, Kraken's cache cleanup should always exclude other services from the disk utilization calculation. I don’t see a scenario where including other services’ disk utilization would improve the cache cleanup behavior.
There was a problem hiding this comment.
The biggest reason is backward compatibility. Other than that, I agree that we don't need the option.
| @@ -14,6 +14,7 @@ | |||
| package store | |||
|
|
|||
| import ( | |||
| // "errors" | |||
| wantTTL: 30 * time.Minute, | ||
| }, | ||
| } { | ||
| require := require.New(t) |
There was a problem hiding this comment.
we usually do test tables like this:
for tt := range tests {
t.Run(tt.desc, func (t *testing.T) {
// test body.
})
}
the require := require.New(t) shouldn't be needed
As described in #385, the aggressive cache cleanup must be configurable to only consider how much % disk only Kraken uses, when deciding whether to clean the cache.
exclude_other_servicesconfig option