github-actions[bot] commented on code in PR #65550:
URL: https://github.com/apache/doris/pull/65550#discussion_r3622806941
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -1872,22 +1872,28 @@ int
InstanceRecycler::abort_job_for_related_rowset(const RowsetMetaCloudPB& rows
return -1;
}
- std::string job_id {};
- if (!job_pb.compaction().empty()) {
- for (const auto& c : job_pb.compaction()) {
- if (c.id() == rowset_meta.job_id()) {
- job_id = c.id();
- break;
- }
+ const TabletCompactionJobPB* matched_compaction = nullptr;
+ for (const auto& compaction : job_pb.compaction()) {
+ if (compaction.id() == rowset_meta.job_id()) {
+ matched_compaction = &compaction;
+ break;
}
- } else if (job_pb.has_schema_change()) {
- job_id = job_pb.schema_change().id();
+ }
+ const TabletSchemaChangeJobPB* matched_schema_change = nullptr;
+ if (matched_compaction == nullptr && job_pb.has_schema_change() &&
+ job_pb.schema_change().id() == rowset_meta.job_id()) {
+ matched_schema_change = &job_pb.schema_change();
}
- if (!job_id.empty() && rowset_meta.job_id() == job_id) {
+ if (matched_compaction != nullptr || matched_schema_change != nullptr) {
LOG(INFO) << "begin to abort job for related rowset, job_id=" <<
rowset_meta.job_id()
<< " instance_id=" << instance_id_ << " tablet_id=" <<
tablet_idx.tablet_id();
- req.mutable_job()->CopyFrom(job_pb);
+ req.mutable_job()->mutable_idx()->CopyFrom(tablet_idx);
Review Comment:
[P1] Preserve the schema-change base tablet index
Schema-change output rowsets use the new tablet, so this lookup reads the
mirrored job record under the new-tablet key. `start_schema_change_job()`
copies the base record into that mirror without changing `job_pb.idx`, but this
line overwrites it with the new-tablet index. The resulting request has
`tablet_id == schema_change.new_tablet_idx().tablet_id()`, so
`process_schema_change_job()` rejects it before its ABORT block. This breaks
even active, unexpired schema-change cleanup (including the existing
`AbortSchemaChangeJobForRelatedRowsetTest2` flow). Build the exact payload with
the recorded base `job_pb.idx()` and cover clearing both mirrored records for a
new-tablet output rowset.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -5178,6 +5140,36 @@ int InstanceRecycler::recycle_rowsets() {
<< " creation_time=" << rowset_meta->creation_time()
<< " task_type=" << metrics_context.operation_type;
if (rowset.type() == RecycleRowsetPB::PREPARE) {
+ if (config::enable_mark_delete_rowset_before_recycle) {
Review Comment:
[P1] Apply this PREPARE barrier to versioned recycling
`recycle_rowsets()` returns to `recycle_versioned_rowsets()` before reaching
this branch when multi-version recycling is enabled. That path's PREPARE branch
deletes by prefix without marking `is_recycled` or aborting the txn/job owner,
although `prepare_rowset()`/`commit_rowset()` still use this same current
recycle key. Because rowset recycling and timeout abort run concurrently (and
force-immediate recycling can select an active owner), a late `commit_rowset()`
can consume the unmarked key and create tmp/ref-count metadata while object
deletion runs; the txn/job can then publish a rowset whose files are gone.
Route versioned PREPARE entries through the same mark, exact-owner abort, and
only-then-delete barrier, and add a multi-version late-commit test.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -1898,6 +1904,44 @@ int InstanceRecycler::abort_job_for_related_rowset(const
RowsetMetaCloudPB& rows
<< " msg=" << msg;
return -1;
}
+
+ std::unique_ptr<Transaction> verify_txn;
+ err = txn_kv_->create_txn(&verify_txn);
+ if (err != TxnErrorCode::TXN_OK) {
+ LOG(WARNING) << "failed to create transaction to verify aborted
job, instance_id="
+ << instance_id_ << " tablet_id=" <<
tablet_idx.tablet_id()
+ << " err=" << err;
+ return -1;
+ }
+ std::string verify_job_val;
+ err = verify_txn->get(job_key, &verify_job_val);
+ if (err == TxnErrorCode::TXN_OK) {
+ TabletJobInfoPB verify_job;
+ if (!verify_job.ParseFromString(verify_job_val)) {
+ LOG(WARNING) << "failed to parse job while verifying abort,
instance_id="
+ << instance_id_ << " tablet_id=" <<
tablet_idx.tablet_id()
+ << " key=" << hex(job_key);
+ return -1;
+ }
+ bool target_job_exists = false;
+ for (const auto& compaction : verify_job.compaction()) {
+ target_job_exists = target_job_exists || compaction.id() ==
rowset_meta.job_id();
+ }
+ target_job_exists = target_job_exists ||
Review Comment:
[P2] Run clang-format v16 on this expression
The required live `Clang Formatter` check is failing, and `clang-format-16
--dry-run --Werror` reports the changed assignment at lines 1930-1931. Please
apply the repository's v16 formatting and rerun the style gate.
--
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]
