github-actions[bot] commented on code in PR #69847:
URL: https://github.com/apache/doris/pull/69847#discussion_r4413060938
##########
regression-test/suites/cloud_p0/cache/compaction/test_stale_rowset.groovy:
##########
@@ -171,13 +171,20 @@ suite("test_stale_rowset") {
select count(*) from ${tableName};
"""
- sleep(60000);
- def backendIdToAfterCompactionCacheSize = getCurCacheSize()
- for (String[] backend in backends) {
- if (backend[9].equals("true") &&
backend[19].contains("regression_cluster_name1")) {
- assertTrue(backendIdToAfterLoadCacheSize.get(backend[0]) >
- backendIdToAfterCompactionCacheSize.get(backend[0]))
+ // The cache of the input rowsets is removed only when the BE sweeps the
stale rowsets, and the
+ // file_cache_cache_size metric is refreshed every few seconds after that.
So poll until it
+ // shrinks instead of reading it once after a fixed wait.
+ def backendIdToAfterCompactionCacheSize = [:]
+ def shrunk = false
+ long deadline = System.currentTimeMillis() + 180000
Review Comment:
[P1] The 180s deadline is shorter than the cloud stale-rowset retention.
`regression-test/pipeline/cloud_p0/conf/be_custom.conf` sets
`tablet_rowset_stale_sweep_time_sec=300`; cloud compaction stamps input rowsets
with the current `stale_at`, and cleanup only selects them after that age. The
pipeline's `vacuum_stale_rowsets_interval_seconds` key is not the declared `_s`
setting, so the effective vacuum interval remains the 300s default. This
assertion therefore expires before cleanup can lower `file_cache_cache_size`.
Derive the deadline from the active stale-sweep settings (with margin) or
configure a shorter sweep for this case.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_lru_evict.groovy:
##########
@@ -232,22 +232,52 @@ suite("test_ttl_lru_evict") {
long ttl_cache_evict_size_begin =
getMetricValues("ttl_cache_evict_size")[0]
long ttl_cache_size_begin = getMetricValues("ttl_cache_size")[0]
- long cache_capacity = getMetricValues("file_cache_capacity").sum()
+ def path_capacities = getMetricValues("file_cache_capacity")
+ // The capacity is reset for all cache paths at once, so it can only be
restored when they
+ // have the same capacity.
+ assertEquals(1, path_capacities.unique(false).size(),
+ "cache paths have different capacities: ${path_capacities}")
+ long path_capacity = path_capacities[0]
+ long cache_capacity = path_capacity
+
+ // Sets the capacity of every cache path of the BE.
+ def resetCapacity = { long capacity ->
+ def (code, out, err) = curl("GET", ("http://"; +
backendIdToBackendIP.get(backendId) + ":" +
+ backendIdToBackendHttpPort.get(backendId) +
+ "/api/file_cache?op=reset&capacity=${capacity}").toString())
+ assertEquals(0, code)
+ assertEquals([capacity] * path_capacities.size(),
getMetricValues("file_cache_capacity"))
Review Comment:
[P1] If the reset succeeds but the metric assertion here fails,
`cache_capacity` is never updated, so `onFinish` believes no shrink happened
and skips restoration. The reset endpoint reports HTTP success even when the
factory can partially mutate paths, and the regression framework swallows
finish-callback exceptions. A transient metric/read failure can therefore leave
the BE at the reduced capacity for subsequent suites. Record that a reset was
attempted before fallible checks and make finish cleanup restore and verify the
original capacity.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_lru_evict.groovy:
##########
@@ -232,22 +232,52 @@ suite("test_ttl_lru_evict") {
long ttl_cache_evict_size_begin =
getMetricValues("ttl_cache_evict_size")[0]
long ttl_cache_size_begin = getMetricValues("ttl_cache_size")[0]
- long cache_capacity = getMetricValues("file_cache_capacity").sum()
+ def path_capacities = getMetricValues("file_cache_capacity")
+ // The capacity is reset for all cache paths at once, so it can only be
restored when they
+ // have the same capacity.
+ assertEquals(1, path_capacities.unique(false).size(),
Review Comment:
[P2] Doris permits each file-cache path to have its own `total_size`, but
this assertion aborts the suite whenever supported capacities differ. The reset
endpoint applies one capacity to all paths, so preserve the original per-path
capacity vector and reset each path explicitly (or skip the shrink optimization
for unequal paths) instead of rejecting the configuration.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_lru_evict.groovy:
##########
@@ -232,22 +232,52 @@ suite("test_ttl_lru_evict") {
long ttl_cache_evict_size_begin =
getMetricValues("ttl_cache_evict_size")[0]
long ttl_cache_size_begin = getMetricValues("ttl_cache_size")[0]
- long cache_capacity = getMetricValues("file_cache_capacity").sum()
+ def path_capacities = getMetricValues("file_cache_capacity")
+ // The capacity is reset for all cache paths at once, so it can only be
restored when they
+ // have the same capacity.
+ assertEquals(1, path_capacities.unique(false).size(),
+ "cache paths have different capacities: ${path_capacities}")
+ long path_capacity = path_capacities[0]
+ long cache_capacity = path_capacity
+
Review Comment:
[P2] The eviction baseline is captured before this capacity reset, but
shrinking a cache removes blocks through `BlockFileCache::remove`, which
increments `ttl_cache_evict_size` for TTL blocks. Those forced removals are
included in the later delta and can satisfy the 228MB assertion before
full-cache pressure eviction occurs. Take the baseline after the capacity
adjustment, or exclude reset-induced removals, before asserting active TTL
eviction.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_lru_evict.groovy:
##########
@@ -232,22 +232,52 @@ suite("test_ttl_lru_evict") {
long ttl_cache_evict_size_begin =
getMetricValues("ttl_cache_evict_size")[0]
long ttl_cache_size_begin = getMetricValues("ttl_cache_size")[0]
- long cache_capacity = getMetricValues("file_cache_capacity").sum()
+ def path_capacities = getMetricValues("file_cache_capacity")
+ // The capacity is reset for all cache paths at once, so it can only be
restored when they
+ // have the same capacity.
+ assertEquals(1, path_capacities.unique(false).size(),
+ "cache paths have different capacities: ${path_capacities}")
+ long path_capacity = path_capacities[0]
+ long cache_capacity = path_capacity
+
+ // Sets the capacity of every cache path of the BE.
+ def resetCapacity = { long capacity ->
+ def (code, out, err) = curl("GET", ("http://"; +
backendIdToBackendIP.get(backendId) + ":" +
Review Comment:
[P2] `getMetricValues` returns one TTL metric per cache path, but the load
budget and later `evicted` delta use only `[0]`. File hashes are routed across
cache instances by hash modulo the cache count, so a load can populate and
evict different amounts on each path. The first path can have too little data
for the 228MB threshold (or zero bytes), causing a false failure while other
paths overflow. Sum the path metrics, or join values by path, for the baseline,
load size, and eviction assertion.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_lru_evict.groovy:
##########
@@ -232,22 +232,52 @@ suite("test_ttl_lru_evict") {
long ttl_cache_evict_size_begin =
getMetricValues("ttl_cache_evict_size")[0]
long ttl_cache_size_begin = getMetricValues("ttl_cache_size")[0]
- long cache_capacity = getMetricValues("file_cache_capacity").sum()
+ def path_capacities = getMetricValues("file_cache_capacity")
+ // The capacity is reset for all cache paths at once, so it can only be
restored when they
+ // have the same capacity.
+ assertEquals(1, path_capacities.unique(false).size(),
+ "cache paths have different capacities: ${path_capacities}")
+ long path_capacity = path_capacities[0]
+ long cache_capacity = path_capacity
Review Comment:
[P2] The baseline is read immediately after `clearFileCache`, but
synchronous block removal does not refresh `ttl_cache_size`; that bvar is
updated only by the background monitor. After the added sleep, the first
post-load value is fresh while `ttl_cache_size_begin` can still be the previous
cache's nonzero value, so `bytes_per_load` is undercounted (or the `> 0`
assertion fails) and this new reset uses an invalid budget. Wait for a monitor
refresh after clearing before taking the baseline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
