github-actions[bot] commented on code in PR #68090:
URL: https://github.com/apache/doris/pull/68090#discussion_r4032450093
##########
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:
[P2] Use the same exact-deadline predicate as the TTL manager. This helper
returns 0 when `creation_time + ttl == UnixSeconds()`, and the new unit test
calls equality expired, but `TtlInfo::is_ttl_active()` still uses `>=` and
calls the same tablet active. During that second direct admission creates
NORMAL blocks while reconciliation can still target TTL, contradicting the
promised single definition and allowing an unnecessary promote/demote edge.
Please share one boundary rule (the new test implies `deadline > now`) and
cover the helper and manager together. This is distinct from the existing
stale-snapshot thread: it occurs even when both decisions use the same current
second.
##########
be/src/storage/rowset/rowset_writer_context.h:
##########
@@ -127,7 +127,11 @@ struct RowsetWriterContext {
/// begin file cache opts
bool write_file_cache = false;
bool is_hot_data = false;
- uint64_t file_cache_ttl_sec = 0;
+ // Absolute timestamp (seconds since epoch) after which the cache blocks
written by
+ // this rowset stop being TTL protected; 0 means no TTL. Always set it from
+ // BaseTablet::file_cache_ttl_expiration_time() so every writer agrees
with the
+ // deadline BlockFileCacheTtlMgr sweeps by.
+ uint64_t file_cache_expiration_time = 0;
Review Comment:
[P2] Thread this deadline through the index-only segment preloader too. When
`enable_file_cache_write_index_file_only` is set, `SegmentFlusher` and
`VerticalBetaRowsetWriter` dry-read embedded footer/index ranges through
`SegmentIndexFileCacheLoader` after close. That loader copies the tablet id but
has no expiration member, then creates an `IOContext` with `is_index_data=true`
and expiration 0, so active-TTL load/schema-change/compaction output is
admitted to INDEX rather than TTL despite this context being populated. Carry
the value into the loader context and assert the stored block metadata for live
and expired TTL tablets.
##########
be/src/cloud/cloud_rowset_builder.cpp:
##########
@@ -107,7 +107,7 @@ Status CloudRowsetBuilder::init() {
context.write_file_cache = _req.write_file_cache;
context.partial_update_info = _partial_update_info;
context.write_binlog_opt().enable = _req.write_req_type ==
WriteRequestType::ROW_BINLOG;
- context.file_cache_ttl_sec = _tablet->ttl_seconds();
+ context.file_cache_expiration_time =
_tablet->file_cache_ttl_expiration_time();
Review Comment:
[P2] Cover transient cloud rowset writers as well. This assignment
initializes the original load writer, but publish-phase partial-update/upsert
conflict repair creates a fresh context in
`CloudTablet::create_transient_rowset_writer()` and leaves
`file_cache_expiration_time` at 0. With
`enable_file_cache_write_index_file_only`,
`get_file_writer_options(INVERTED_INDEX_FILE)` still forces those generated
index files into cache, so an active-TTL tablet gets NORMAL entries. Initialize
the deadline centrally for every cloud data-producing writer, including
attached row-binlog transient writers, and test a conflict-generated segment
under index-only mode.
##########
regression-test/suites/cloud_p0/cache/ttl/test_ttl_expired_tablet.groovy:
##########
@@ -0,0 +1,207 @@
+// 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 and query paths must stamp the
blocks they create as
+// non-TTL right away, instead of creating TTL blocks for the background sweep
to demote again.
+//
+// This test deliberately runs with the TTL background threads turned down to
a 10 minute
+// interval. That is what gives it teeth: the sweep would otherwise repair the
cache type within
+// a second or two and the assertions would hold no matter what the write and
read paths did.
+// With the sweep out of the way, the cache type observed here is purely the
one chosen at
+// admission, so the expired-tablet cases below fail if the deadline is not
applied at the source.
+suite("test_ttl_expired_tablet") {
+ def ttlSeconds = 30
+ def sweepOffMs = 600000
+
+ def custoBeConfig = [
+ enable_evict_file_cache_in_advance : false,
+ file_cache_enter_disk_resource_limit_mode_percent : 99,
+ // Long enough that neither TTL background thread reconciles anything
while the test runs.
+ file_cache_background_ttl_gc_interval_ms : sweepOffMs,
Review Comment:
[P2] Avoid pausing these process-wide threads with a ten-minute sleep. Each
TTL loop samples this dynamic value and enters `std::this_thread::sleep_for()`;
restoring the config in `setBeConfigTemporary` does not wake it, and
`BlockFileCacheTtlMgr::stop()` joins the same sleeping threads. Depending on
loop phase, this test can therefore leave reconciliation disabled for later
parallel TTL suites or delay BE shutdown for most of ten minutes; an old
sampled interval can also allow one reconciliation during this test. Use an
interruptible test-scoped pause or a synchronization hook that is explicitly
released before leaving the closure.
##########
be/src/storage/rowset/beta_rowset_reader.cpp:
##########
@@ -242,7 +242,7 @@ Status
BetaRowsetReader::get_segment_iterators(RowsetReaderContext* read_context
_read_options.condition_cache_digest =
_read_context->condition_cache_digest;
}
- _read_options.io_ctx.expiration_time = read_context->ttl_seconds;
+ _read_options.io_ctx.expiration_time =
read_context->file_cache_expiration_time;
Review Comment:
[P2] Preserve this context on both eager reader branches. First, the V1
`IndexFileReader::_open()` path constructs `DorisCompoundReader` without the
supplied `io_ctx`; its constructor immediately reads the cold compound
directory with expiration 0, unlike V2/SNII. Second, the `record_rowids &&
rowid_conversion` branch below calls `get_segment_num_rows()` without its
supported context argument, so legacy rowsets cold-read their footer with
expiration 0 before iterators exist. Pass the context through both boundaries
and add cold V1 predicate/score plus compaction row-ID coverage. The V1 loss is
downstream of the already-raised score-producer fix and also affects ordinary
predicates.
##########
be/src/cloud/cloud_schema_change_job.cpp:
##########
@@ -405,7 +405,7 @@ Status
CloudSchemaChangeJob::_convert_historical_rowsets(const SchemaChangeParam
// like the load and compaction output does. Otherwise it is cached in
the
// NORMAL/INDEX queues here, while every warm-up path downloads it
into the TTL
// queue on the destination cluster.
- context.file_cache_ttl_sec = _new_tablet->ttl_seconds();
+ context.file_cache_expiration_time =
_new_tablet->file_cache_ttl_expiration_time();
Review Comment:
[P2] Propagate the base tablet's deadline to schema-change input reads too.
`process_alter_tablet()` builds a manual `RowsetReaderContext` for
`READER_ALTER_TABLE` but never sets `file_cache_expiration_time`;
sorting/type-changing conversion then cold-reads the base rowsets with
expiration 0, so data pages are admitted DISPOSABLE and footer/index blocks
INDEX while that tablet's TTL is active. This assignment only fixes new-tablet
outputs and cannot repair those source blocks. Initialize the reader context
from `_base_tablet` and cover cold source metadata/data admission.
##########
be/src/storage/tablet/base_tablet.h:
##########
@@ -78,6 +78,10 @@ class BaseTablet : public
std::enable_shared_from_this<BaseTablet> {
KeysType keys_type() const { return
_tablet_meta->tablet_schema()->keys_type(); }
size_t num_key_columns() const { return
_tablet_meta->tablet_schema()->num_key_columns(); }
int64_t ttl_seconds() const { return _tablet_meta->ttl_seconds(); }
+ // See TabletMeta::file_cache_ttl_expiration_time().
+ int64_t file_cache_ttl_expiration_time() const {
Review Comment:
[P2] Use this deadline for publish-phase MoW reads as well as writers.
`update_delete_bitmap()`/`calc_segment_delete_bitmap()` currently load the
incoming segment's PK/Bloom-filter data with a null context, then perform
historical lookup and `FixedReadPlan` old-value fetches without an expiration
context. On an active-TTL tablet those cold misses are admitted as INDEX or
NORMAL, independently of the transient-output omission. Create one
expiration-bearing publish `IOContext` and thread it through segment loading,
lookup, and old-value reads; cover conflict repair on a cold cache.
##########
be/src/storage/compaction/compaction.cpp:
##########
@@ -2383,7 +2383,7 @@ Status
CloudCompactionMixin::construct_output_rowset_writer(RowsetWriterContext&
// TODO(gavin): Ensure that the retention of hot data is implemented with
precision.
ctx.write_file_cache = should_cache_compaction_output();
- ctx.file_cache_ttl_sec = _tablet->ttl_seconds();
+ ctx.file_cache_expiration_time = _tablet->file_cache_ttl_expiration_time();
Review Comment:
[P2] Propagate the deadline to compaction input index reads, not only the
output writer. Index eligibility runs before this assignment, and both it and
`do_inverted_index_compaction()` construct source `IndexFileReader`s whose
`init`/`open`/`open_snii_index` calls use a null `IOContext`. Cold V2/SNII
container metadata and V1 compound reads therefore reach the cached reader as
index data with expiration 0, admitting INDEX blocks for an active-TTL tablet.
Build an expiration-bearing compaction context for every source read and add
cold-cache metadata coverage across the supported formats.
--
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]
