github-actions[bot] commented on code in PR #68090:
URL: https://github.com/apache/doris/pull/68090#discussion_r4026755619
##########
be/src/cloud/cloud_warm_up_manager.cpp:
##########
@@ -297,7 +297,7 @@ void CloudWarmUpManager::handle_jobs() {
continue;
}
- int64_t expiration_time = tablet_meta->ttl_seconds();
+ int64_t expiration_time =
tablet_meta->file_cache_ttl_expiration_time();
Review Comment:
[P1] Preserve the tablet id on the synchronous warm-up path. These warm-ups
are forced through `CachedRemoteFileReader::_read_from_indirect_cache()`, which
constructs `CacheContext(io_ctx)` but never copies `_tablet_id` (unlike the
async and remote-only paths). The resulting blocks are stored with tablet_id 0,
so `BlockFileCacheTtlMgr` cannot find or register them and a block warmed
before the deadline can remain TTL indefinitely. Set the tablet id before
`get_or_set()` and cover a warm-up that starts live and is observed after
expiry.
##########
be/src/storage/tablet/tablet_reader.cpp:
##########
@@ -191,7 +191,7 @@ Status TabletReader::_capture_rs_readers(const
ReaderParams& read_params) {
_reader_context.output_columns = &read_params.output_columns;
_reader_context.extra_columns = read_params.extra_columns;
_reader_context.push_down_agg_type_opt =
read_params.push_down_agg_type_opt;
- _reader_context.ttl_seconds = _tablet->ttl_seconds();
+ _reader_context.file_cache_expiration_time =
_tablet->file_cache_ttl_expiration_time();
Review Comment:
[P2] Update the parallel score-runtime read context too.
`OlapScanner::_prepare_impl()` still calls
`build_score_runtime_collection_io_context(..., tablet->ttl_seconds(), ...)`;
the helper copies that argument directly to `IOContext::expiration_time`, and
`CollectionStatistics` carries it through `IndexFileReader` into cached remote
reads. Scoring cache misses therefore still persist `3600` instead of
`creation_time + 3600` and can recreate TTL blocks for an expired tablet. Pass
the absolute tablet deadline here as well and update the olap-scanner tests.
##########
be/src/cloud/cloud_internal_service.cpp:
##########
@@ -1270,7 +1270,7 @@ void
CloudInternalServiceImpl::warm_up_rowset(google::protobuf::RpcController* c
<< " us, tablet_id: " << rs_meta.tablet_id()
<< ", rowset_id: " << rowset_id.to_string();
}
- int64_t expiration_time = tablet_meta->ttl_seconds();
+ int64_t expiration_time =
tablet_meta->file_cache_ttl_expiration_time();
Review Comment:
[P2] Carry the absolute deadline through peer-server pull-through fills too.
The detached peer race calls `fetch_blocks()` without its IO context, and the
request carries tablet/resource identity but no expiration. On the fill server,
`handle_peer_file_cache_block_request()` therefore performs the first
`get_or_set()` with a default context, creating a NORMAL block owned by tablet
0 before the downloader runs; the later synchronous read only finds that
existing cell and cannot replace its metadata. Active-TTL tablets filled
through the designated fill compute group consequently bypass TTL protection.
Propagate both fields into the first admission and capability-gate or fail
closed when an old server cannot honor TTL-aware fill; then test the stored
metadata and field-absent fallback, not only returned bytes.
##########
be/src/io/fs/file_writer.h:
##########
@@ -47,7 +47,7 @@ struct FileWriterOptions {
bool allow_adaptive_file_cache_write = true;
bool is_cold_data = false;
bool sync_file_data = true; // Whether flush data into
storage system
- uint64_t file_cache_expiration_time = 0; // Relative time
+ uint64_t file_cache_expiration_time = 0; // Absolute time, 0 means no TTL
Review Comment:
[P1] Migrate and durably normalize existing expiration metadata. This field
already exists on disk and across BE warm-up RPCs, but older blocks contain
durations such as `3600`. Startup restores them before `BlockFileCacheTtlMgr`
exists, `add_cell()` turns every nonzero value into TTL, and the restored
tablet ids are never registered. Even blocks demoted by the manager keep their
nonzero expiration, so a subsequent DB-driven restore can put them back in TTL.
Please migrate/validate restored and peer-provided values against the owning
tablet, register restored tablet ids, and rewrite expiration together with
queue type so convergence survives a restart.
##########
be/src/storage/tablet/tablet_meta.cpp:
##########
@@ -716,6 +716,25 @@ Status TabletMeta::save_meta(DataDir* data_dir) {
return _save_meta(data_dir);
}
+int64_t TabletMeta::file_cache_ttl_expiration_time() const {
+ int64_t ttl = ttl_seconds();
+ int64_t ctime = creation_time();
+ if (ttl <= 0 || ctime <= 0) {
+ return 0;
+ }
+ // FE caps file_cache_ttl_seconds at Long.MAX_VALUE / 2, so this cannot
wrap, but a tablet
+ // meta that reached us from anywhere else still must not turn a huge ttl
into a past
+ // deadline that silently downgrades the tablet to normal cache.
+ if (ctime > std::numeric_limits<int64_t>::max() - ttl) {
+ return std::numeric_limits<int64_t>::max();
+ }
+ int64_t expiration_time = ctime + ttl;
+ // Already past the deadline: report no TTL at all, so callers stamp the
blocks they
+ // create as NORMAL right away instead of putting them in the TTL queue for
+ // BlockFileCacheTtlMgr to take straight back out again.
+ return expiration_time > UnixSeconds() ? expiration_time : 0;
Review Comment:
[P1] Re-evaluate the deadline at final cache admission. This helper is
sampled once and every caller stores the result, but `CacheContext`,
`FileCacheAllocatorBuilder`, and packed-file admission treat any nonzero value
as TTL without comparing it to the current time. A load or queued warm-up
started just before expiry therefore keeps creating TTL blocks after the
deadline, recreating the sweep churn; a context that captured zero can likewise
keep creating NORMAL blocks after an ALTER extension, after the manager's
edge-triggered promotion has already finished. Normalize/reload the current
tablet deadline when each block is admitted and add deterministic
cross-expiry/ALTER tests.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_expired_tablet.groovy:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.codehaus.groovy.runtime.IOGroovyMethods
+
+// The file cache TTL deadline is the tablet creation time plus
file_cache_ttl_seconds.
+// Once a tablet is past that deadline, the load, query and warm up paths must
all stamp the
+// blocks they create as NORMAL right away. Before the deadline was defined in
one place,
+// those paths passed the raw ttl_seconds instead, so every block went into
the TTL queue and
+// the background sweep pulled it straight back out, over and over, for the
rest of the
+// tablet's life. This test pins that down: past the deadline, nothing reaches
the TTL queue.
+suite("test_ttl_expired_tablet") {
+ def custoBeConfig = [
+ enable_evict_file_cache_in_advance : false,
+ file_cache_enter_disk_resource_limit_mode_percent : 99,
+ file_cache_background_ttl_gc_interval_ms : 1000,
+ file_cache_background_ttl_info_update_interval_ms : 1000,
+ file_cache_background_tablet_id_flush_interval_ms : 1000
+ ]
+
+ setBeConfigTemporary(custoBeConfig) {
+ sql "set global enable_auto_analyze = false"
+ sql "set global enable_audit_plugin = false"
+ def clusters = sql " SHOW CLUSTERS; "
+ assertTrue(!clusters.isEmpty())
+ def validCluster = clusters[0][0]
+ sql """use @${validCluster};""";
+
+ def ttlSeconds = 30
+ def ttlProperties = """
PROPERTIES("file_cache_ttl_seconds"="${ttlSeconds}") """
+ String[][] backends = sql """ show backends """
+ String backendId;
+ def backendIdToBackendIP = [:]
+ def backendIdToBackendHttpPort = [:]
+ def backendIdToBackendBrpcPort = [:]
+ for (String[] backend in backends) {
+ if (backend[9].equals("true") &&
backend[19].contains("regression_cluster_name1")) {
+ backendIdToBackendIP.put(backend[0], backend[1])
+ backendIdToBackendHttpPort.put(backend[0], backend[4])
+ backendIdToBackendBrpcPort.put(backend[0], backend[5])
+ }
+ }
+ assertEquals(backendIdToBackendIP.size(), 1)
+
+ backendId = backendIdToBackendIP.keySet()[0]
+ def url = backendIdToBackendIP.get(backendId) + ":" +
backendIdToBackendHttpPort.get(backendId) +
"""/api/file_cache?op=clear&sync=true"""
+ def clearFileCache = { check_func ->
+ httpTest {
+ endpoint ""
+ uri url
+ op "get"
+ body ""
+ check check_func
+ }
+ }
+
+ def getMetricsMethod = { check_func ->
+ httpTest {
+ endpoint backendIdToBackendIP.get(backendId) + ":" +
backendIdToBackendBrpcPort.get(backendId)
+ uri "/brpc_metrics"
+ op "get"
+ check check_func
+ }
+ }
+
+ def getTtlCacheSize = {
+ long ttlCacheSize = -1
+ getMetricsMethod.call() {
+ respCode, body ->
+ assertEquals("${respCode}".toString(), "200")
+ String out = "${body}".toString()
+ for (String line in out.split('\n')) {
+ if (line.startsWith("#")) {
+ continue
+ }
+ if (line.contains("ttl_cache_size")) {
+ def i = line.indexOf(' ')
+ ttlCacheSize = line.substring(i).toLong()
+ break
+ }
+ }
+ }
+ assertTrue(ttlCacheSize >= 0, "ttl_cache_size metric not found")
+ return ttlCacheSize
+ }
+
+ def getTabletIds = { String tableName ->
+ def tablets = sql "show tablets from ${tableName}"
+ assertTrue(tablets.size() > 0, "No tablets found for table
${tableName}")
+ tablets.collect { it[0] as Long }
+ }
+
+ def waitForFileCacheType = { List<Long> tabletIds, String expectedType,
long timeoutMs = 60000L, long intervalMs = 1000L ->
+ long start = System.currentTimeMillis()
+ while (System.currentTimeMillis() - start < timeoutMs) {
+ boolean allMatch = true
+ for (Long tabletId in tabletIds) {
+ def rows = sql "select type from
information_schema.file_cache_info where tablet_id = ${tabletId}"
+ if (rows.isEmpty()) {
+ allMatch = false
+ break
+ }
+ def mismatch = rows.find { row ->
!row[0]?.toString()?.equalsIgnoreCase(expectedType) }
+ if (mismatch) {
+ logger.info("tablet ${tabletId} has cache types
${rows.collect { it[0] }} while waiting for ${expectedType}")
+ allMatch = false
+ break
+ }
+ }
+ if (allMatch) {
+ return
+ }
+ sleep(intervalMs)
+ }
+ assertTrue(false, "Timeout waiting for file_cache_info type
${expectedType} for tablets ${tabletIds}")
+ }
+
+ def loadCustomerRows = { String table ->
+ def totalRows = 200
+ def batchSize = 100
+ def commentSuffix = ' ' + ('X' * 50)
+ for (int offset = 0; offset < totalRows; offset += batchSize) {
+ def sb = new StringBuilder()
+ int batchEnd = Math.min(totalRows, offset + batchSize)
+ for (int idx = offset; idx < batchEnd; idx++) {
+ def customerId = 10001 + idx
+ def customerName = String.format('Customer#%09d', customerId)
+ sb.append("""INSERT INTO ${table} VALUES (
+ ${customerId},
+ '${customerName}',
+ 'Address Line 1',
+ 15,
+ '123-456-7890',
+ 12345.67,
+ 'AUTOMOBILE',
+ 'This is a test comment for the customer.${commentSuffix}'
+ );
+ """)
+ }
+ sql sb.toString()
+ }
+ }
+
+ sql new
File("""${context.file.parent}/../ddl/customer_ttl_delete.sql""").text
+ clearFileCache.call() {
+ respCode, body -> {}
+ }
+ sleep(10000)
+ assertEquals(0L, getTtlCacheSize.call())
+
+ // Create the table, then let its TTL deadline pass before writing a
single row.
+ sql (new File("""${context.file.parent}/../ddl/customer_ttl.sql""").text +
ttlProperties)
+ sql """ alter table customer_ttl set ("disable_auto_compaction" = "true")
"""
+ sleep((ttlSeconds + 15) * 1000L)
+
+ loadCustomerRows("customer_ttl")
+ def tabletIds = getTabletIds.call("customer_ttl")
+
+ // The tablet is past its deadline, so the load path must have written
every block into
Review Comment:
[P2] Make direct cache admission observable here. With the old code the
expired tablet first admits these blocks as TTL, but the one-second background
reconciler can demote them during the 60-second wait, so the load assertions
still pass after the production fix is reverted. The subsequent SELECT is also
a cache hit and never exercises the changed reader context. Please prevent or
pause reconciliation while checking the load insertion, then clear and verify
the cache is empty before SELECT and assert that the read recreates NORMAL
blocks; add an actual warm-up case for the warm-up changes.
--
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]
