Skip to content

Forbid creating empty tiering rules in column shards #8154

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

Merged
swalrus1 merged 8 commits into from
Aug 23, 2024
Merged

Forbid creating empty tiering rules in column shards #8154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9387513
Forbid creating empty tiering rules in column shards
swalrus1 Aug 22, 2024
547dad9
don't crash when rules are empty
swalrus1 Aug 22, 2024
a0ab22c
improve UT
swalrus1 Aug 22, 2024
de6bfcb
clean up
swalrus1 Aug 22, 2024
a5733ef
remove HasTiers check
swalrus1 Aug 22, 2024
2fe0763
Add a VERIFY
swalrus1 Aug 22, 2024
0774229
post-review fixes
swalrus1 Aug 23, 2024
e4a401f
add defaultColumn validation
swalrus1 Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions ydb/core/kqp/ut/olap/tiering_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,70 @@ Y_UNIT_TEST_SUITE(KqpOlapTiering) {
<< " after=" << GetUint64(rows[0].at("RawBytes")));
}
}

Y_UNIT_TEST(TieringRuleValidation) {
auto csController = NYDBTest::TControllers::RegisterCSControllerGuard<NOlap::TWaitCompactionController>();

TKikimrSettings runnerSettings;
runnerSettings.WithSampleTables = false;
TTestHelper testHelper(runnerSettings);
TLocalHelper localHelper(testHelper.GetKikimr());
NYdb::NTable::TTableClient tableClient = testHelper.GetKikimr().GetTableClient();
Tests::NCommon::TLoggerInit(testHelper.GetKikimr()).Initialize();
Singleton<NKikimr::NWrappers::NExternalStorage::TFakeExternalStorage>()->SetSecretKey("fakeSecret");

localHelper.CreateTestOlapTable();
testHelper.CreateTier("tier1");

{
const TString query = R"(
CREATE OBJECT IF NOT EXISTS empty_tiering_rule (TYPE TIERING_RULE)
WITH (defaultColumn = timestamp, description = `{"rules": []}`))";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}

{
const TString query = R"(
CREATE OBJECT IF NOT EXISTS empty_default_column (TYPE TIERING_RULE)
WITH (defaultColumn = ``, description = `{"rules": [{ "tierName" : "tier1", "durationForEvict" : "10d" }]}`))";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}

{
const TString query = R"(
CREATE OBJECT IF NOT EXISTS no_default_column (TYPE TIERING_RULE)
WITH (description = `{"rules": [{ "tierName" : "tier1", "durationForEvict" : "10d" }]}`))";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}

const TString correctTieringRule = testHelper.CreateTieringRule("tier1", "timestamp");
{
const TString query = "ALTER OBJECT " + correctTieringRule + R"( (TYPE TIERING_RULE) SET description `{"rules": []}`)";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}

{
const TString query = "ALTER OBJECT " + correctTieringRule + R"( (TYPE TIERING_RULE) SET description `{"rules": []}`)";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}

{
const TString query = "ALTER OBJECT " + correctTieringRule + R"( (TYPE TIERING_RULE) SET defaultColumn ``)";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}

{
const TString query = "ALTER OBJECT " + correctTieringRule + R"( (TYPE TIERING_RULE) RESET defaultColumn)";
auto result = testHelper.GetSession().ExecuteSchemeQuery(query).GetValueSync();
UNIT_ASSERT_VALUES_UNEQUAL(result.GetStatus(), NYdb::EStatus::SUCCESS);
}
}
}

} // namespace NKikimr::NKqp
19 changes: 10 additions & 9 deletions ydb/core/tx/columnshard/engines/scheme/tiering/tier_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class TTiering {
using TTiersMap = THashMap<TString, std::shared_ptr<TTierInfo>>;
TTiersMap TierByName;
TSet<TTierRef> OrderedTiers;
TString TTLColumnName;
std::optional<TString> TTLColumnName;
public:

class TTieringContext {
Expand Down Expand Up @@ -174,9 +174,14 @@ class TTiering {
[[nodiscard]] bool Add(const std::shared_ptr<TTierInfo>& tier) {
AFL_VERIFY(tier);
if (!TTLColumnName) {
if (tier->GetEvictColumnName().Empty()) {
AFL_ERROR(NKikimrServices::TX_COLUMNSHARD)("problem", "empty_evict_column_name");
return false;
}
TTLColumnName = tier->GetEvictColumnName();
Copy link
Collaborator

@ivanmorozov333 ivanmorozov333 Aug 23, 2024

Choose a reason for hiding this comment

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

добавить проверку на не пустую строку

} else if (TTLColumnName != tier->GetEvictColumnName()) {
AFL_ERROR(NKikimrServices::TX_COLUMNSHARD)("problem", "incorrect_tiering_metadata")("column_before", TTLColumnName)("column_new", tier->GetEvictColumnName());
} else if (*TTLColumnName != tier->GetEvictColumnName()) {
AFL_ERROR(NKikimrServices::TX_COLUMNSHARD)("problem", "incorrect_tiering_metadata")("column_before", *TTLColumnName)
("column_new", tier->GetEvictColumnName());
return false;
}

Expand All @@ -194,13 +199,9 @@ class TTiering {
return {};
}

const TString& GetTtlColumn() const {
AFL_VERIFY(TTLColumnName);
return TTLColumnName;
}

const TString& GetEvictColumnName() const {
return TTLColumnName;
AFL_VERIFY(TTLColumnName);
return *TTLColumnName;
}

TString GetDebugString() const {
Expand Down
2 changes: 1 addition & 1 deletion ydb/core/tx/columnshard/engines/storage/actualizer/tiering/tiering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ void TTieringActualizer::DoExtractTasks(TTieringProcessContext& tasksContext, co
void TTieringActualizer::Refresh(const std::optional<TTiering>& info, const TAddExternalContext& externalContext) {
Tiering = info;
if (Tiering) {
TieringColumnId = VersionedIndex.GetLastSchema()->GetColumnId(Tiering->GetTtlColumn());
TieringColumnId = VersionedIndex.GetLastSchema()->GetColumnId(Tiering->GetEvictColumnName());
} else {
TieringColumnId = {};
}
Expand Down
19 changes: 9 additions & 10 deletions ydb/core/tx/tiering/manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,19 +199,18 @@ THashMap<ui64, NKikimr::NOlap::TTiering> TTiersManager::GetTiering() const {
Y_ABORT_UNLESS(snapshotPtr);
auto& tierConfigs = snapshotPtr->GetTierConfigs();
for (auto&& i : PathIdTiering) {
auto* tiering = snapshotPtr->GetTieringById(i.second);
if (tiering) {
auto* tieringRule = snapshotPtr->GetTieringById(i.second);
if (tieringRule) {
AFL_DEBUG(NKikimrServices::TX_COLUMNSHARD)("path_id", i.first)("tiering_name", i.second)("event", "activation");
result.emplace(i.first, tiering->BuildOlapTiers());
for (auto& [pathId, pathTiering] : result) {
for (auto& [name, tier] : pathTiering.GetTierByName()) {
AFL_VERIFY(name != NOlap::NTiering::NCommon::DeleteTierName);
auto it = tierConfigs.find(name);
if (it != tierConfigs.end()) {
tier->SetSerializer(NTiers::ConvertCompression(it->second.GetCompression()));
}
NOlap::TTiering tiering = tieringRule->BuildOlapTiers();
for (auto& [name, tier] : tiering.GetTierByName()) {
AFL_VERIFY(name != NOlap::NTiering::NCommon::DeleteTierName);
auto it = tierConfigs.find(name);
if (it != tierConfigs.end()) {
tier->SetSerializer(NTiers::ConvertCompression(it->second.GetCompression()));
}
}
result.emplace(i.first, std::move(tiering));
} else {
AFL_ERROR(NKikimrServices::TX_COLUMNSHARD)("path_id", i.first)("tiering_name", i.second)("event", "not_found");
}
Expand Down
3 changes: 3 additions & 0 deletions ydb/core/tx/tiering/rule/manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ NMetadata::NModifications::TOperationParsingResult TTieringRulesManager::DoBuild
{
auto fValue = settings.GetFeaturesExtractor().Extract(TTieringRule::TDecoder::DefaultColumn);
if (fValue) {
if (fValue->Empty()) {
return TConclusionStatus::Fail("defaultColumn cannot be empty");
}
result.SetColumn(TTieringRule::TDecoder::DefaultColumn, NMetadata::NInternal::TYDBValue::Utf8(*fValue));
}
}
Expand Down
8 changes: 8 additions & 0 deletions ydb/core/tx/tiering/rule/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ bool TTieringRule::DeserializeDescriptionFromJson(const NJson::TJsonValue& jsonI
if (!jsonInfo["rules"].GetArrayPointer(&rules)) {
return false;
}
if (rules->empty()) {
AFL_INFO(NKikimrServices::TX_COLUMNSHARD)("event", "tiering_rule_deserialization_failed")("reason", "empty_rules");
return false;
}
for (auto&& i : *rules) {
TTieringInterval interval;
if (!interval.DeserializeFromJson(i)) {
Expand Down Expand Up @@ -61,6 +65,9 @@ bool TTieringRule::DeserializeFromRecord(const TDecoder& decoder, const Ydb::Val
if (!decoder.Read(decoder.GetDefaultColumnIdx(), DefaultColumn, r)) {
return false;
}
if (DefaultColumn.Empty()) {
return false;
}
NJson::TJsonValue jsonDescription;
if (!decoder.ReadJson(decoder.GetDescriptionIdx(), jsonDescription, r)) {
return false;
Expand All @@ -72,6 +79,7 @@ bool TTieringRule::DeserializeFromRecord(const TDecoder& decoder, const Ydb::Val
}

NKikimr::NOlap::TTiering TTieringRule::BuildOlapTiers() const {
AFL_VERIFY(!Intervals.empty());
NOlap::TTiering result;
for (auto&& r : Intervals) {
AFL_VERIFY(result.Add(std::make_shared<NOlap::TTierInfo>(r.GetTierName(), r.GetDurationForEvict(), GetDefaultColumn())));
Expand Down