Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix 00002_log_and_exception_messages_formatting #61882

Merged
merged 1 commit into from Apr 8, 2024
Merged

Fix 00002_log_and_exception_messages_formatting #61882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Storages/MergeTree/MergeTreeData.cpp
View file
Open in desktop
Expand Up @@ -7773,11 +7773,11 @@ MovePartsOutcome MergeTreeData::moveParts(const CurrentlyMovingPartsTaggerPtr &
return result;
}

bool MergeTreeData::partsContainSameProjections(const DataPartPtr & left, const DataPartPtr & right, String & out_reason)
bool MergeTreeData::partsContainSameProjections(const DataPartPtr & left, const DataPartPtr & right, PreformattedMessage & out_reason)
{
if (left->getProjectionParts().size() != right->getProjectionParts().size())
{
out_reason = fmt::format(
out_reason = PreformattedMessage::create(
"Parts have different number of projections: {} in part '{}' and {} in part '{}'",
left->getProjectionParts().size(),
left->name,
Expand All @@ -7791,7 +7791,7 @@ bool MergeTreeData::partsContainSameProjections(const DataPartPtr & left, const
{
if (!right->hasProjection(name))
{
out_reason = fmt::format(
out_reason = PreformattedMessage::create(
"The part '{}' doesn't have projection '{}' while part '{}' does", right->name, name, left->name
);
return false;
Expand Down
2 changes: 1 addition & 1 deletion src/Storages/MergeTree/MergeTreeData.h
View file
Open in desktop
Expand Up @@ -418,7 +418,7 @@ class MergeTreeData : public IStorage, public WithMutableContext
static ReservationPtr tryReserveSpace(UInt64 expected_size, const IDataPartStorage & data_part_storage);
static ReservationPtr reserveSpace(UInt64 expected_size, const IDataPartStorage & data_part_storage);

static bool partsContainSameProjections(const DataPartPtr & left, const DataPartPtr & right, String & out_reason);
static bool partsContainSameProjections(const DataPartPtr & left, const DataPartPtr & right, PreformattedMessage & out_reason);

StoragePolicyPtr getStoragePolicy() const override;

Expand Down
34 changes: 17 additions & 17 deletions src/Storages/MergeTree/MergeTreeDataMergerMutator.cpp
View file
Open in desktop
Expand Up @@ -136,7 +136,7 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectPartsToMerge(
const AllowedMergingPredicate & can_merge_callback,
bool merge_with_ttl_allowed,
const MergeTreeTransactionPtr & txn,
String & out_disable_reason,
PreformattedMessage & out_disable_reason,
const PartitionIdsHint * partitions_hint)
{
MergeTreeData::DataPartsVector data_parts = getDataPartsToSelectMergeFrom(txn, partitions_hint);
Expand All @@ -145,15 +145,15 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectPartsToMerge(

if (data_parts.empty())
{
out_disable_reason = "There are no parts in the table";
out_disable_reason = PreformattedMessage::create("There are no parts in the table");
return SelectPartsDecision::CANNOT_SELECT;
}

MergeSelectingInfo info = getPossibleMergeRanges(data_parts, can_merge_callback, txn, out_disable_reason);

if (info.parts_selected_precondition == 0)
{
out_disable_reason = "No parts satisfy preconditions for merge";
out_disable_reason = PreformattedMessage::create("No parts satisfy preconditions for merge");
return SelectPartsDecision::CANNOT_SELECT;
}

Expand All @@ -177,9 +177,9 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectPartsToMerge(
/*optimize_skip_merged_partitions=*/true);
}

if (!out_disable_reason.empty())
out_disable_reason += ". ";
out_disable_reason += "There is no need to merge parts according to merge selector algorithm";
if (!out_disable_reason.text.empty())
out_disable_reason.text += ". ";
out_disable_reason.text += "There is no need to merge parts according to merge selector algorithm";
return SelectPartsDecision::CANNOT_SELECT;
}

Expand All @@ -196,7 +196,7 @@ MergeTreeDataMergerMutator::PartitionIdsHint MergeTreeDataMergerMutator::getPart

auto metadata_snapshot = data.getInMemoryMetadataPtr();

String out_reason;
PreformattedMessage out_reason;
MergeSelectingInfo info = getPossibleMergeRanges(data_parts, can_merge_callback, txn, out_reason);

if (info.parts_selected_precondition == 0)
Expand All @@ -223,7 +223,7 @@ MergeTreeDataMergerMutator::PartitionIdsHint MergeTreeDataMergerMutator::getPart
for (size_t i = 0; i < all_partition_ids.size(); ++i)
{
auto future_part = std::make_shared<FutureMergedMutatedPart>();
String out_disable_reason;
PreformattedMessage out_disable_reason;
/// This method should have been const, but something went wrong... it's const with dry_run = true
auto status = const_cast<MergeTreeDataMergerMutator *>(this)->selectPartsToMergeFromRanges(
future_part, /*aggressive*/ false, max_total_size_to_merge, merge_with_ttl_allowed,
Expand All @@ -232,7 +232,7 @@ MergeTreeDataMergerMutator::PartitionIdsHint MergeTreeDataMergerMutator::getPart
if (status == SelectPartsDecision::SELECTED)
res.insert(all_partition_ids[i]);
else
LOG_TEST(log, "Nothing to merge in partition {}: {}", all_partition_ids[i], out_disable_reason);
LOG_TEST(log, "Nothing to merge in partition {}: {}", all_partition_ids[i], out_disable_reason.text);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now format_string from out_disable_reason is ignored here. But we discussed how can it be improved in the next PRs.

}

String best_partition_id_to_optimize = getBestPartitionToOptimizeEntire(info.partitions_info);
Expand Down Expand Up @@ -331,7 +331,7 @@ MergeTreeDataMergerMutator::MergeSelectingInfo MergeTreeDataMergerMutator::getPo
const MergeTreeData::DataPartsVector & data_parts,
const AllowedMergingPredicate & can_merge_callback,
const MergeTreeTransactionPtr & txn,
String & out_disable_reason) const
PreformattedMessage & out_disable_reason) const
{
MergeSelectingInfo res;

Expand Down Expand Up @@ -444,7 +444,7 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectPartsToMergeFromRanges(
const StorageMetadataPtr & metadata_snapshot,
const IMergeSelector::PartsRanges & parts_ranges,
const time_t & current_time,
String & out_disable_reason,
PreformattedMessage & out_disable_reason,
bool dry_run)
{
const auto data_settings = data.getSettings();
Expand Down Expand Up @@ -515,7 +515,7 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectPartsToMergeFromRanges(

if (parts_to_merge.empty())
{
out_disable_reason = "Did not find any parts to merge (with usual merge selectors)";
out_disable_reason = PreformattedMessage::create("Did not find any parts to merge (with usual merge selectors)");
return SelectPartsDecision::CANNOT_SELECT;
}
}
Expand Down Expand Up @@ -573,20 +573,20 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectAllPartsToMergeWithinParti
bool final,
const StorageMetadataPtr & metadata_snapshot,
const MergeTreeTransactionPtr & txn,
String & out_disable_reason,
PreformattedMessage & out_disable_reason,
bool optimize_skip_merged_partitions)
{
MergeTreeData::DataPartsVector parts = selectAllPartsFromPartition(partition_id);

if (parts.empty())
{
out_disable_reason = "There are no parts inside partition";
out_disable_reason = PreformattedMessage::create("There are no parts inside partition");
return SelectPartsDecision::CANNOT_SELECT;
}

if (!final && parts.size() == 1)
{
out_disable_reason = "There is only one part inside partition";
out_disable_reason = PreformattedMessage::create("There is only one part inside partition");
return SelectPartsDecision::CANNOT_SELECT;
}

Expand All @@ -595,7 +595,7 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectAllPartsToMergeWithinParti
if (final && optimize_skip_merged_partitions && parts.size() == 1 && parts[0]->info.level > 0 &&
(!metadata_snapshot->hasAnyTTL() || parts[0]->checkAllTTLCalculated(metadata_snapshot)))
{
out_disable_reason = "Partition skipped due to optimize_skip_merged_partitions";
out_disable_reason = PreformattedMessage::create("Partition skipped due to optimize_skip_merged_partitions");
return SelectPartsDecision::NOTHING_TO_MERGE;
}

Expand Down Expand Up @@ -636,7 +636,7 @@ SelectPartsDecision MergeTreeDataMergerMutator::selectAllPartsToMergeWithinParti
static_cast<int>((DISK_USAGE_COEFFICIENT_TO_SELECT - 1.0) * 100));
}

out_disable_reason = fmt::format("Insufficient available disk space, required {}", ReadableSize(required_disk_space));
out_disable_reason = PreformattedMessage::create("Insufficient available disk space, required {}", ReadableSize(required_disk_space));
return SelectPartsDecision::CANNOT_SELECT;
}

Expand Down
10 changes: 5 additions & 5 deletions src/Storages/MergeTree/MergeTreeDataMergerMutator.h
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@ class MergeTreeDataMergerMutator
using AllowedMergingPredicate = std::function<bool (const MergeTreeData::DataPartPtr &,
const MergeTreeData::DataPartPtr &,
const MergeTreeTransaction *,
String &)>;
PreformattedMessage &)>;

explicit MergeTreeDataMergerMutator(MergeTreeData & data_);

Expand Down Expand Up @@ -92,7 +92,7 @@ class MergeTreeDataMergerMutator
const MergeTreeData::DataPartsVector & data_parts,
const AllowedMergingPredicate & can_merge_callback,
const MergeTreeTransactionPtr & txn,
String & out_disable_reason) const;
PreformattedMessage & out_disable_reason) const;

/// The third step of selecting parts to merge: takes ranges that we can merge, and selects parts that we want to merge
SelectPartsDecision selectPartsToMergeFromRanges(
Expand All @@ -103,7 +103,7 @@ class MergeTreeDataMergerMutator
const StorageMetadataPtr & metadata_snapshot,
const IMergeSelector::PartsRanges & parts_ranges,
const time_t & current_time,
String & out_disable_reason,
PreformattedMessage & out_disable_reason,
bool dry_run = false);

String getBestPartitionToOptimizeEntire(const PartitionsInfo & partitions_info) const;
Expand All @@ -129,7 +129,7 @@ class MergeTreeDataMergerMutator
const AllowedMergingPredicate & can_merge,
bool merge_with_ttl_allowed,
const MergeTreeTransactionPtr & txn,
String & out_disable_reason,
PreformattedMessage & out_disable_reason,
const PartitionIdsHint * partitions_hint = nullptr);

/** Select all the parts in the specified partition for merge, if possible.
Expand All @@ -144,7 +144,7 @@ class MergeTreeDataMergerMutator
bool final,
const StorageMetadataPtr & metadata_snapshot,
const MergeTreeTransactionPtr & txn,
String & out_disable_reason,
PreformattedMessage & out_disable_reason,
bool optimize_skip_merged_partitions = false);

/** Creates a task to merge parts.
Expand Down
41 changes: 20 additions & 21 deletions src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp
View file
Open in desktop
Expand Up @@ -2266,7 +2266,7 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::operator()(
const MergeTreeData::DataPartPtr & left,
const MergeTreeData::DataPartPtr & right,
const MergeTreeTransaction *,
String & out_reason) const
PreformattedMessage & out_reason) const
{
if (left)
return canMergeTwoParts(left, right, out_reason);
Expand All @@ -2278,7 +2278,7 @@ template<typename VirtualPartsT, typename MutationsStateT>
bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
const MergeTreeData::DataPartPtr & left,
const MergeTreeData::DataPartPtr & right,
String & out_reason) const
PreformattedMessage & out_reason) const
{
/// A sketch of a proof of why this method actually works:
///
Expand Down Expand Up @@ -2322,19 +2322,19 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
{
if (pinned_part_uuids_ && pinned_part_uuids_->part_uuids.contains(part->uuid))
{
out_reason = "Part " + part->name + " has uuid " + toString(part->uuid) + " which is currently pinned";
out_reason = PreformattedMessage::create("Part {} has uuid {} which is currently pinned", part->name, part->uuid);
return false;
}

if (inprogress_quorum_part_ && part->name == *inprogress_quorum_part_)
{
out_reason = "Quorum insert for part " + part->name + " is currently in progress";
out_reason = PreformattedMessage::create("Quorum insert for part {} is currently in progress", part->name);
return false;
}

if (prev_virtual_parts_ && prev_virtual_parts_->getContainingPart(part->info).empty())
{
out_reason = "Entry for part " + part->name + " hasn't been read from the replication log yet";
out_reason = PreformattedMessage::create("Entry for part {} hasn't been read from the replication log yet", part->name);
return false;
}
}
Expand All @@ -2348,7 +2348,7 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
{
if (partition_ids_hint && !partition_ids_hint->contains(left->info.partition_id))
{
out_reason = fmt::format("Uncommitted block were not loaded for unexpected partition {}", left->info.partition_id);
out_reason = PreformattedMessage::create("Uncommitted block were not loaded for unexpected partition {}", left->info.partition_id);
return false;
}

Expand All @@ -2360,8 +2360,7 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
auto block_it = block_numbers.upper_bound(left_max_block);
if (block_it != block_numbers.end() && *block_it < right_min_block)
{
out_reason = "Block number " + toString(*block_it) + " is still being inserted between parts "
+ left->name + " and " + right->name;
out_reason = PreformattedMessage::create("Block number {} is still being inserted between parts {} and {}", *block_it, left->name, right->name);
return false;
}
}
Expand All @@ -2380,7 +2379,7 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
String containing_part = virtual_parts_->getContainingPart(part->info);
if (containing_part != part->name)
{
out_reason = "Part " + part->name + " has already been assigned a merge into " + containing_part;
out_reason = PreformattedMessage::create("Part {} has already been assigned a merge into {}", part->name, containing_part);
return false;
}
}
Expand All @@ -2397,9 +2396,9 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
Strings covered = virtual_parts_->getPartsCoveredBy(gap_part_info);
if (!covered.empty())
{
out_reason = "There are " + toString(covered.size()) + " parts (from " + covered.front()
+ " to " + covered.back() + ") that are still not present or being processed by "
+ " other background process on this replica between " + left->name + " and " + right->name;
out_reason = PreformattedMessage::create("There are {} parts (from {} to {}) "
"that are still not present or being processed by other background process "
"on this replica between {} and {}", covered.size(), covered.front(), covered.back(), left->name, right->name);
return false;
}
}
Expand All @@ -2415,8 +2414,8 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(

if (left_mutation_ver != right_mutation_ver)
{
out_reason = "Current mutation versions of parts " + left->name + " and " + right->name + " differ: "
+ toString(left_mutation_ver) + " and " + toString(right_mutation_ver) + " respectively";
out_reason = PreformattedMessage::create("Current mutation versions of parts {} and {} differ: "
"{} and {} respectively", left->name, right->name, left_mutation_ver, right_mutation_ver);
return false;
}
}
Expand All @@ -2427,23 +2426,23 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeTwoParts(
template<typename VirtualPartsT, typename MutationsStateT>
bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeSinglePart(
const MergeTreeData::DataPartPtr & part,
String & out_reason) const
PreformattedMessage & out_reason) const
{
if (pinned_part_uuids_ && pinned_part_uuids_->part_uuids.contains(part->uuid))
{
out_reason = fmt::format("Part {} has uuid {} which is currently pinned", part->name, part->uuid);
out_reason = PreformattedMessage::create("Part {} has uuid {} which is currently pinned", part->name, part->uuid);
return false;
}

if (inprogress_quorum_part_ && part->name == *inprogress_quorum_part_)
{
out_reason = fmt::format("Quorum insert for part {} is currently in progress", part->name);
out_reason = PreformattedMessage::create("Quorum insert for part {} is currently in progress", part->name);
return false;
}

if (prev_virtual_parts_ && prev_virtual_parts_->getContainingPart(part->info).empty())
{
out_reason = fmt::format("Entry for part {} hasn't been read from the replication log yet", part->name);
out_reason = PreformattedMessage::create("Entry for part {} hasn't been read from the replication log yet", part->name);
return false;
}

Expand All @@ -2458,7 +2457,7 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeSinglePart(
String containing_part = virtual_parts_->getContainingPart(part->info);
if (containing_part != part->name)
{
out_reason = fmt::format("Part {} has already been assigned a merge into {}", part->name, containing_part);
out_reason = PreformattedMessage::create("Part {} has already been assigned a merge into {}", part->name, containing_part);
return false;
}
}
Expand All @@ -2467,7 +2466,7 @@ bool BaseMergePredicate<VirtualPartsT, MutationsStateT>::canMergeSinglePart(
}


bool ReplicatedMergeTreeMergePredicate::partParticipatesInReplaceRange(const MergeTreeData::DataPartPtr & part, String & out_reason) const
bool ReplicatedMergeTreeMergePredicate::partParticipatesInReplaceRange(const MergeTreeData::DataPartPtr & part, PreformattedMessage & out_reason) const
{
std::lock_guard lock(queue.state_mutex);
for (const auto & entry : queue.queue)
Expand All @@ -2480,7 +2479,7 @@ bool ReplicatedMergeTreeMergePredicate::partParticipatesInReplaceRange(const Mer
if (part->info.isDisjoint(MergeTreePartInfo::fromPartName(part_name, queue.format_version)))
continue;

out_reason = fmt::format("Part {} participates in REPLACE_RANGE {} ({})", part_name, entry->new_part_name, entry->znode_name);
out_reason = PreformattedMessage::create("Part {} participates in REPLACE_RANGE {} ({})", part_name, entry->new_part_name, entry->znode_name);
return true;
}
}
Expand Down