github-actions[bot] commented on code in PR #67088:
URL: https://github.com/apache/doris/pull/67088#discussion_r3849508319
##########
be/src/storage/rowset/rowset_meta_manager.cpp:
##########
@@ -84,21 +89,108 @@ Status RowsetMetaManager::save(OlapMeta* meta, TabletUid
tablet_uid, const Rowse
}
DBUG_EXECUTE_IF("RowsetMetaManager::save::zero_partition_id", {
long partition_id = rowset_meta_pb.partition_id();
- auto& rs_pb =
const_cast<std::decay_t<decltype(rowset_meta_pb)>&>(rowset_meta_pb);
- rs_pb.set_partition_id(0);
+ rowset_meta_pb.set_partition_id(0);
LOG(WARNING) << "set debug point
RowsetMetaManager::save::zero_partition_id old="
<< partition_id << " new=" <<
rowset_meta_pb.DebugString();
});
+ if (rowset_meta.need_persist_schema()) {
Review Comment:
[P1] Guarantee the schema key at the persistence boundary. This PB is
stripped at line 82, but this branch trusts a flag captured before
`_build_current_tablet_schema()` can advance the tablet's in-memory maximum. If
the first writer for version N advances that maximum and aborts, a second
writer for N records `need_persist_schema()==false` and can commit a rowset
with neither an inline schema nor an `rs_..._N` key;
`PushHandler::_convert_v2()` has the same default-false path. After a crash,
`DataDir::load()` cannot recover that committed rowset and CCR schema export
fails. Please idempotently persist or verify the exact schema before every
stripped save, and cover the abort/commit ordering in a restart test.
##########
be/src/storage/rowset/rowset_meta_manager.cpp:
##########
@@ -84,21 +89,108 @@ Status RowsetMetaManager::save(OlapMeta* meta, TabletUid
tablet_uid, const Rowse
}
DBUG_EXECUTE_IF("RowsetMetaManager::save::zero_partition_id", {
long partition_id = rowset_meta_pb.partition_id();
- auto& rs_pb =
const_cast<std::decay_t<decltype(rowset_meta_pb)>&>(rowset_meta_pb);
- rs_pb.set_partition_id(0);
+ rowset_meta_pb.set_partition_id(0);
LOG(WARNING) << "set debug point
RowsetMetaManager::save::zero_partition_id old="
<< partition_id << " new=" <<
rowset_meta_pb.DebugString();
});
+ if (rowset_meta.need_persist_schema()) {
+ RETURN_IF_ERROR(save_schema(meta, rowset_meta.tablet_id(), tablet_uid,
+ rowset_meta.tablet_schema_hash(),
tablet_schema));
+ }
if (!binlog_format.has_value()) {
return _save(meta, tablet_uid, rowset_id, rowset_meta_pb);
}
if (*binlog_format == BinlogFormatPB::STATEMENT_AND_SNAPSHOT) {
return _save_with_ccr_binlog(meta, tablet_uid, rowset_id,
rowset_meta_pb);
}
- DCHECK_EQ(*binlog_format, BinlogFormatPB::ROW);
- DCHECK(attach_row_binlog_rowset_meta.has_value());
+ DORIS_CHECK(*binlog_format == BinlogFormatPB::ROW);
+ DORIS_CHECK(attach_row_binlog_rowset_meta != nullptr);
+ const auto& attach_row_binlog_tablet_schema =
attach_row_binlog_rowset_meta->tablet_schema();
+ DORIS_CHECK(attach_row_binlog_tablet_schema != nullptr);
+ if (attach_row_binlog_rowset_meta->need_persist_schema()) {
+ RETURN_IF_ERROR(save_schema(meta,
attach_row_binlog_rowset_meta->tablet_id(),
+
attach_row_binlog_rowset_meta->tablet_uid(),
+
attach_row_binlog_rowset_meta->tablet_schema_hash(),
+ attach_row_binlog_tablet_schema));
+ }
+ RowsetMetaPB attach_row_binlog_rowset_pb =
attach_row_binlog_rowset_meta->get_rowset_pb(true);
return _save_with_row_binlog(meta, tablet_uid, rowset_id, rowset_meta_pb,
- *attach_row_binlog_rowset_meta);
+ attach_row_binlog_rowset_pb);
+}
+
+bool RowsetMetaManager::schema_exists(OlapMeta* meta, TabletUid tablet_uid,
int32_t schema_hash,
+ int32_t schema_version) {
+ std::string schema_key = fmt::format("{}{}_{}_{}", ROWSET_SCHEMA_PREFIX,
tablet_uid.to_string(),
+ schema_hash, schema_version);
+ std::string value;
+ return meta->key_may_exist(META_COLUMN_FAMILY_INDEX, schema_key, &value) &&
+ meta->get(META_COLUMN_FAMILY_INDEX, schema_key, &value).ok();
+}
+
+Status RowsetMetaManager::save_schema(OlapMeta* meta, TTabletId tablet_id,
TabletUid tablet_uid,
+ int32_t schema_hash, const
TabletSchemaSPtr& schema) {
+ DORIS_CHECK(schema != nullptr);
+ // Variant rowsets keep their rowset-specific schemas inline.
+ if (schema->num_variant_columns() > 0) {
+ return Status::OK();
+ }
+ const int32_t schema_version = schema->schema_version();
+ std::string schema_key = fmt::format("{}{}_{}_{}", ROWSET_SCHEMA_PREFIX,
tablet_uid.to_string(),
+ schema_hash, schema_version);
+ if (schema_exists(meta, tablet_uid, schema_hash, schema_version)) {
Review Comment:
[P1] Do not use schema version as the rowset-schema identity. Online
inverted/ANN index add and drop copy a rowset schema, change its index
metadata, and replace the rowset without incrementing `schema_version`. On an
already migrated tablet this early return therefore preserves the pre-change
bytes, while the replacement rowset is saved without its inline schema; after
restart it is reconstructed with the wrong index metadata. Please reference an
exact content identity or retain divergent schemas inline, and add restart
coverage for both index add and drop.
##########
be/test/storage/path_gc_test.cpp:
##########
@@ -158,7 +158,7 @@ TEST(PathGcTest, GcTabletAndRowset) {
st = create_rowset_files(*rs, false);
ASSERT_TRUE(st.ok()) << st;
st = RowsetMetaManager::save(data_dir.get_meta(),
rs->rowset_meta()->tablet_uid(),
- rs->rowset_id(),
rs->rowset_meta()->get_rowset_pb(), false);
+ rs->rowset_id(), *rs->rowset_meta());
Review Comment:
[P1] Populate the `RowsetMeta` schema before calling the new save API.
`create_rowset()` passes the tablet schema only to `BetaRowset`; the `Rowset`
constructor stores that fallback in `Rowset::_schema` and leaves
`rowset_meta()->tablet_schema()` null. `RowsetMetaManager::save()` now
immediately `DORIS_CHECK`s that pointer, so this test aborts during setup
instead of exercising path GC.
##########
be/src/storage/tablet/tablet_manager.cpp:
##########
@@ -956,6 +959,12 @@ Status TabletManager::load_tablet_from_meta(DataDir*
data_dir, TTabletId tablet_
RETURN_NOT_OK_STATUS_WITH_WARN(
tablet->init(), absl::Substitute("tablet init failed. tablet=$0",
tablet->tablet_id()));
+ // Clone and restore replace the tablet uid. Backfill schemas missing from
legacy rowset metas
+ // before _add_tablet_unlocked() persists them under the new uid via
TabletMeta::_save_meta().
+ if (need_persist_schema) {
Review Comment:
[P2] Reclaim the replaced UID's schema prefix during force restore. This
path generates a new tablet UID and persists its schemas, but
`_add_tablet_unlocked(..., force=true)` drops the old tablet with
`keep_files=true`; that old object is not queued for `_move_tablet_to_trash()`,
the only normal path that calls `remove_schemas()`. Repeated restores therefore
retain every old `rs_<uid>_...` prefix indefinitely. Please remove the old
UID's keys after replacement is durable, or add an ownership-proven
orphan-schema sweep and a repeated-restore test.
##########
be/src/storage/tablet/tablet.cpp:
##########
@@ -475,6 +481,12 @@ Status Tablet::revise_tablet_meta(const
std::vector<RowsetSharedPtr>& to_add,
}
}
+ for (const auto& rowset : to_add) {
Review Comment:
[P1] Persist these schemas before mutating the live tablet, or roll the
mutation back on failure. Incremental clone has already called
`add_rowsets(to_add)`, and full clone has already deleted and added its
rowsets, before this new RocksDB operation can return an error.
`_finish_clone()` then deletes the newly linked files whenever that error
propagates, leaving the in-memory tablet referencing rowsets whose segment
files are gone; full clone has also discarded the old map. Please move the
fallible persistence ahead of both mutation branches or provide complete
rollback coverage.
##########
be/src/storage/tablet/tablet_manager.cpp:
##########
@@ -1345,7 +1386,7 @@ bool TabletManager::_move_tablet_to_trash(const
TabletSharedPtr& tablet) {
<< "tablet_id=" << tablet->tablet_id()
<< ", schema_hash=" << tablet->schema_hash()
<< ", tablet_path=" << tablet_path;
- return true;
+ return !check_st.is<META_KEY_NOT_FOUND>() ||
remove_separated_schemas();
Review Comment:
[P1] Do not treat this status as proof that the tablet header is absent.
`TabletMetaManager::get_meta()` can read `tabletmeta_` successfully and then
return `META_KEY_NOT_FOUND` from the new deserializer when `ts_` or a
referenced `rs_` key is missing. This branch, and the path-present branch
above, then removes only side keys or the path and returns true, leaving the
stripped header while dequeuing the shutdown tablet. The next startup traverses
that header, fails before reading its shutdown state, and is fatal by default.
Please distinguish raw header absence and delete the header plus proven side
keys with retriable error handling.
##########
be/src/storage/data_dir.cpp:
##########
@@ -654,10 +652,10 @@ Status DataDir::load() {
rowset_meta->tablet_uid() == tablet->tablet_uid()) {
if (!rowset_meta->tablet_schema()) {
rowset_meta->set_tablet_schema(tablet->tablet_schema());
- RETURN_IF_ERROR(RowsetMetaManager::save(_meta,
rowset_meta->tablet_uid(),
-
rowset_meta->rowset_id(),
-
rowset_meta->get_rowset_pb(), binlog_format,
-
attach_row_binlog_rowset_meta));
+ rowset_meta->set_persist_schema(true);
Review Comment:
[P1] Persist inline recovered schemas too. During upgrade, `DataDir::load()`
migrates legacy tablet headers and sets `tablet_schema_saved` before processing
standalone rowsets. A visible old-format `s_` record can already carry a
historical inline schema, for example after a crash between `publish_txn()` and
`add_inc_rowset()`, so this null-only block is skipped. `add_rowset()` then
adds it to the migrated tablet; the next checkpoint strips its inline schema
and deletes `s_`, but no `rs_` key was written, making the following restart
fail. Please save or verify every recovered visible schema before adding it,
and cover upgrade plus checkpoint plus restart.
##########
be/src/storage/tablet/tablet_meta_manager.cpp:
##########
@@ -123,6 +174,12 @@ Status TabletMetaManager::remove(DataDir* store, TTabletId
tablet_id, TSchemaHas
OlapMeta* meta = store->get_meta();
Status res = meta->remove(META_COLUMN_FAMILY_INDEX, key);
VLOG_NOTICE << "remove tablet_meta, key:" << key << ", res:" << res;
+ if (res.ok() && header_prefix == HEADER_PREFIX) {
Review Comment:
[P2] Remove the UID-scoped rowset schemas as part of this deletion contract.
`meta_tool`'s single and batch delete commands call
`TabletMetaManager::remove()` directly, but a successful call now deletes only
the header and `ts_` key; no caller removes `rs_<uid>_<hash>_...`, and no
background sweep scans that prefix. Once this header is gone, the UID needed to
identify those keys is also lost, so every administrative deletion leaks them
permanently. Please read or prove the UID and remove its prefix before erasing
the header, with single- and batch-delete coverage.
##########
be/src/storage/tablet/tablet_meta.cpp:
##########
@@ -712,10 +714,35 @@ Status TabletMeta::_save_meta(DataDir* data_dir) {
LOG(FATAL) << "tablet_uid is invalid"
<< " tablet=" << tablet_id() << " _tablet_uid=" <<
_tablet_uid.to_string();
}
+
+ if (!_tablet_schema_saved) {
+ std::map<int32_t, TabletSchemaSPtr> rowset_schemas;
+ for (const auto& [_, rowset_meta] : _rs_metas) {
+ DORIS_CHECK(rowset_meta->tablet_schema() != nullptr);
+ rowset_schemas[rowset_meta->tablet_schema()->schema_version()] =
+ rowset_meta->tablet_schema();
+ }
+ for (const auto& [_, rowset_meta] : _stale_rs_metas) {
+ DORIS_CHECK(rowset_meta->tablet_schema() != nullptr);
+ rowset_schemas[rowset_meta->tablet_schema()->schema_version()] =
+ rowset_meta->tablet_schema();
+ }
+ for (const auto& [_, schema] : rowset_schemas) {
+
RETURN_IF_ERROR(RowsetMetaManager::save_schema(data_dir->get_meta(),
tablet_id(),
+ tablet_uid(),
schema_hash(), schema));
+ }
+
+ TabletSchemaPB schema_pb;
+ _schema->to_schema_pb(&schema_pb);
+ RETURN_IF_ERROR(TabletMetaManager::save_schema(
+ data_dir, tablet_id(), schema_hash(),
+ TabletSchema::deterministic_string_serialize(schema_pb)));
+ _tablet_schema_saved = true;
+ }
string meta_binary;
auto t1 = MonotonicMicros();
- serialize(&meta_binary);
+ serialize(&meta_binary, true);
Review Comment:
[P1] Preserve rollback readability before writing this stripped format. The
immediately prior BE ignores the new `tablet_schema_saved` field and only reads
`TabletMetaPB.schema` plus each rowset's inline `tablet_schema`; after any
ordinary checkpoint reaches this call, all of those fields are omitted. Rolling
that node back then constructs an empty tablet schema and schema-less rowsets.
Please dual-write during the supported rollback window or guard migration with
an explicitly coordinated format gate, with an old-reader compatibility test.
##########
be/src/storage/rowset/rowset_meta.cpp:
##########
@@ -221,32 +235,40 @@ RowsetMetaPB RowsetMeta::get_rowset_pb(bool skip_schema)
const {
}
void RowsetMeta::set_tablet_schema(const TabletSchemaSPtr& tablet_schema) {
- if (_handle) {
- TabletSchemaCache::instance()->release(_handle);
- }
- auto pair = TabletSchemaCache::instance()->insert(tablet_schema->to_key());
- _handle = pair.first;
- _schema = pair.second;
+ _set_tablet_schema_from_binary(tablet_schema->to_key());
}
void RowsetMeta::set_tablet_schema(const TabletSchemaPB& tablet_schema) {
+
_set_tablet_schema_from_binary(TabletSchema::deterministic_string_serialize(tablet_schema));
+}
+
+void RowsetMeta::_set_tablet_schema_from_binary(const std::string&
schema_binary) {
if (_handle) {
TabletSchemaCache::instance()->release(_handle);
}
- auto pair = TabletSchemaCache::instance()->insert(
- TabletSchema::deterministic_string_serialize(tablet_schema));
+ auto pair = TabletSchemaCache::instance()->insert(schema_binary);
_handle = pair.first;
_schema = pair.second;
}
-bool RowsetMeta::_deserialize_from_pb(std::string_view value) {
+bool RowsetMeta::_deserialize_from_pb(std::string_view value, OlapMeta* meta) {
if (!_rowset_meta_pb.ParseFromArray(value.data(),
cast_set<int32_t>(value.size()))) {
_rowset_meta_pb.Clear();
return false;
}
if (_rowset_meta_pb.has_tablet_schema()) {
set_tablet_schema(_rowset_meta_pb.tablet_schema());
_rowset_meta_pb.set_allocated_tablet_schema(nullptr);
+ } else if (meta != nullptr && _rowset_meta_pb.has_schema_version()) {
+ std::string schema_binary;
+ Status status = RowsetMetaManager::get_rowset_schema(
+ meta, _rowset_meta_pb.tablet_id(),
TabletUid(_rowset_meta_pb.tablet_uid()),
+ _rowset_meta_pb.tablet_schema_hash(),
_rowset_meta_pb.schema_version(),
+ &schema_binary);
+ if (!status.ok()) {
Review Comment:
[P1] Preserve this lookup `Status` instead of reporting every failure as a
parse error. `DataDir::load()` treats `false` as skippable malformed metadata,
so a missing, corrupt, or unreadable schema key silently removes a committed
rowset from recovery; the background unused-meta cleaner treats the same result
as corruption and deletes its standalone record. Please distinguish protobuf
corruption from schema dependency errors and fail or quarantine the load rather
than continuing with a shortened version chain.
##########
be/src/storage/tablet/tablet.cpp:
##########
@@ -566,6 +578,12 @@ Status
Tablet::modify_rowsets(std::vector<RowsetSharedPtr>& to_add,
}
}
+ for (const auto& rowset : to_add) {
+ RETURN_IF_ERROR(RowsetMetaManager::save_schema(data_dir()->get_meta(),
tablet_id(),
Review Comment:
[P2] Keep this RocksDB I/O out of the exclusive header-lock section.
`IndexBuilder` holds `get_header_lock()` while calling `modify_rowsets()` and
creates one output per candidate rowset. This loop calls `save_schema()` per
output; for an already migrated same-version set, `schema_exists()` still
performs `KeyMayExist` plus a synchronous `DB::Get` each time, commonly against
the same key. Large index add/drop operations therefore do O(rowsets) metadata
reads while publish waits. Please deduplicate exact schemas and persist or
batch them before taking the lock, retaining fail-before-mutation ordering.
##########
be/src/storage/rowset/rowset_meta.cpp:
##########
@@ -221,32 +235,40 @@ RowsetMetaPB RowsetMeta::get_rowset_pb(bool skip_schema)
const {
}
void RowsetMeta::set_tablet_schema(const TabletSchemaSPtr& tablet_schema) {
- if (_handle) {
- TabletSchemaCache::instance()->release(_handle);
- }
- auto pair = TabletSchemaCache::instance()->insert(tablet_schema->to_key());
- _handle = pair.first;
- _schema = pair.second;
+ _set_tablet_schema_from_binary(tablet_schema->to_key());
}
void RowsetMeta::set_tablet_schema(const TabletSchemaPB& tablet_schema) {
+
_set_tablet_schema_from_binary(TabletSchema::deterministic_string_serialize(tablet_schema));
+}
+
+void RowsetMeta::_set_tablet_schema_from_binary(const std::string&
schema_binary) {
if (_handle) {
TabletSchemaCache::instance()->release(_handle);
}
- auto pair = TabletSchemaCache::instance()->insert(
- TabletSchema::deterministic_string_serialize(tablet_schema));
+ auto pair = TabletSchemaCache::instance()->insert(schema_binary);
Review Comment:
[P1] Reject malformed schema bytes before inserting them into the cache.
`TabletSchemaCache::insert()` parses this value with
`TabletSchemaPB::ParseFromString()` but ignores the result, so a successful
RocksDB lookup containing malformed bytes produces a non-null empty or partial
`TabletSchema`. Deserialization then returns success, `DataDir`'s null-schema
fallback is bypassed, and restart can recover a committed rowset with an
invalid schema instead of failing. Please propagate a parse `Status` and add
malformed-`rs_` restart coverage.
--
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]
