Skip to content

Commit 68050f5

Browse files
ildar-khisambeevildar-khisambeev
authored
24-1 cherry-pick remove redundant checks (#1417) (#3700)
1 parent 0ef6729 commit 68050f5

File tree

4 files changed

+28
-43
lines changed

4 files changed

+28
-43
lines changed

‎ydb/library/persqueue/topic_parser/topic_parser.cpp

+8-22
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,7 @@ bool TDiscoveryConverter::BuildFromFederationPath(const TString& rootPrefix) {
320320
auto res = topic.TrySplit("/", fst, snd);
321321
CHECK_SET_VALID(res, TStringBuilder() << "Could not split federation path: " << OriginalTopic, return false);
322322
Account_ = fst;
323-
323+
324324
if (!ParseModernPath(snd))
325325
return false;
326326
if (!BuildFromShortModernName()) {
@@ -341,7 +341,7 @@ bool TDiscoveryConverter::BuildFromFederationPath(const TString& rootPrefix) {
341341

342342
bool TDiscoveryConverter::TryParseModernMirroredPath(TStringBuf path) {
343343
if (!path.Contains("-mirrored-from-")) {
344-
CHECK_SET_VALID(!path.Contains("mirrored-from"), "Federation topics cannot contain 'mirrored-from' in name unless this is a mirrored topic", return false);
344+
CHECK_SET_VALID(!path.Contains("mirrored-from"), "Federation topics cannot contain 'mirrored-from' in name unless this is a mirrored topic", return false);
345345
return false;
346346
}
347347
TStringBuf fst, snd;
@@ -447,7 +447,7 @@ bool TDiscoveryConverter::BuildFromLegacyName(const TString& rootPrefix, bool fo
447447
return false);
448448
}
449449
if (Dc.empty() && !hasDcInName) {
450-
CHECK_SET_VALID(!FstClass, TStringBuilder() << "Internal error: FirstClass mode enabled, but trying to parse Legacy-style name: "
450+
CHECK_SET_VALID(!FstClass, TStringBuilder() << "Internal error: FirstClass mode enabled, but trying to parse Legacy-style name: "
451451
<< OriginalTopic, return false;);
452452
CHECK_SET_VALID(!LocalDc.empty(),
453453
"Cannot determine DC: should specify either in topic name, Dc option or LocalDc option",
@@ -466,7 +466,7 @@ bool TDiscoveryConverter::BuildFromLegacyName(const TString& rootPrefix, bool fo
466466
Dc = fst;
467467
topic = snd;
468468
} else {
469-
CHECK_SET_VALID(!Dc.empty(), TStringBuilder() << "Internal error: Could not determine DC (despite beleiving the name contins one) for topic "
469+
CHECK_SET_VALID(!Dc.empty(), TStringBuilder() << "Internal error: Could not determine DC (despite beleiving the name contins one) for topic "
470470
<< OriginalTopic, return false;);
471471
TStringBuilder builder;
472472
builder << "rt3." << Dc << "--" << topic;
@@ -513,7 +513,7 @@ bool TDiscoveryConverter::BuildFromLegacyName(const TString& rootPrefix, bool fo
513513
topicName = topic;
514514
}
515515
modernName << topicName;
516-
CHECK_SET_VALID(!Dc.empty(), TStringBuilder() << "Internal error: Could not determine DC for topic: "
516+
CHECK_SET_VALID(!Dc.empty(), TStringBuilder() << "Internal error: Could not determine DC for topic: "
517517
<< OriginalTopic, return false);
518518

519519
bool isMirrored = (!LocalDc.empty() && Dc != LocalDc);
@@ -522,7 +522,7 @@ bool TDiscoveryConverter::BuildFromLegacyName(const TString& rootPrefix, bool fo
522522
} else {
523523
fullModernName << topicName;
524524
}
525-
CHECK_SET_VALID(!fullLegacyName.empty(), TStringBuilder() << "Could not form a full legacy name for topic: "
525+
CHECK_SET_VALID(!fullLegacyName.empty(), TStringBuilder() << "Could not form a full legacy name for topic: "
526526
<< OriginalTopic, return false);
527527

528528
ShortLegacyName = shortLegacyName;
@@ -669,21 +669,7 @@ TTopicConverterPtr TTopicNameConverter::ForFederation(
669669
if (!res->IsValid()) {
670670
return res;
671671
}
672-
if (parsed) {
673-
Y_ABORT_UNLESS(!res->Dc.empty());
674-
if (!localDc.empty() && localDc == res->Dc) {
675-
res->Valid = false;
676-
res->Reason = TStringBuilder() << "Topic in modern mirrored-like style: " << schemeName
677-
<< " cannot be created in the same cluster " << res->Dc;
678-
return res;
679-
}
680-
}
681672
if (isLocal) {
682-
if(parsed) {
683-
res->Valid = false;
684-
res->Reason = TStringBuilder() << "Topic in modern mirrored-like style: " << schemeName << ", created as local";
685-
return res;
686-
}
687673
if (localDc.empty()) {
688674
res->Valid = false;
689675
res->Reason = "Local DC option is mandatory when creating local modern-style topic";
@@ -694,7 +680,8 @@ TTopicConverterPtr TTopicNameConverter::ForFederation(
694680
if (!ok) {
695681
return res;
696682
}
697-
} else {
683+
}
684+
else {
698685
if (!parsed) {
699686
res->Valid = false;
700687
res->Reason = TStringBuilder() << "Topic in modern style with non-mirrored-name: " << schemeName
@@ -943,4 +930,3 @@ TConverterFactoryPtr TTopicsListController::GetConverterFactory() const {
943930
};
944931

945932
} // namespace NPersQueue
946-

‎ydb/library/persqueue/topic_parser/ut/topic_names_converter_ut.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ Y_UNIT_TEST_SUITE(DiscoveryConverterTest) {
175175
);
176176
UNIT_ASSERT_VALUES_EQUAL(converter->GetFullModernName(), "account/account");
177177
UNIT_ASSERT_VALUES_EQUAL(converter->GetSecondaryPath("account"), "/account/account/account");
178-
178+
179179
converter = converterFactory.MakeDiscoveryConverter(
180180
"account/", {}, "dc1", "account"
181181
);
@@ -431,10 +431,11 @@ Y_UNIT_TEST_SUITE(TopicNameConverterForCPTest) {
431431
);
432432
UNIT_ASSERT(!converter->IsValid());
433433

434+
// this is valid, corresponding check relaxed
434435
converter = TTopicNameConverter::ForFederation(
435436
"", "", "topic-mirrored-from-sas", "/LbCommunal/account", "/LbCommunal/account", true, "sas", "account"
436437
);
437-
UNIT_ASSERT(!converter->IsValid());
438+
UNIT_ASSERT(converter->IsValid());
438439

439440
converter = TTopicNameConverter::ForFederation(
440441
"", "", "topic-mirrored-from-", "/LbCommunal/account", "/LbCommunal/account", true, "sas", "account"

‎ydb/services/persqueue_v1/actors/schema_actors.cpp

+13-17
Original file line numberDiff line numberDiff line change
@@ -396,10 +396,6 @@ void TCreateTopicActor::FillProposeRequest(TEvTxUserProxy::TEvProposeTransaction
396396
<< "' instead of " << LocalCluster, Ydb::PersQueue::ErrorCode::BAD_REQUEST));
397397
return RespondWithCode(Ydb::StatusIds::BAD_REQUEST);
398398
}
399-
if (Count(Clusters, config.GetDC()) == 0 && !Clusters.empty()) {
400-
Request_->RaiseIssue(FillIssue(TStringBuilder() << "Unknown cluster '" << config.GetDC() << "'", Ydb::PersQueue::ErrorCode::BAD_REQUEST));
401-
return RespondWithCode(Ydb::StatusIds::BAD_REQUEST);
402-
}
403399
}
404400

405401

@@ -596,7 +592,7 @@ void TDescribeTopicActorImpl::Handle(TEvPQProxy::TEvRequestTablet::TPtr& ev, con
596592
Y_ABORT_UNLESS(RequestsInfly > 0);
597593
--RequestsInfly;
598594
}
599-
595+
600596
RequestTablet(tabletInfo, ctx);
601597
}
602598

@@ -635,7 +631,7 @@ void TDescribeTopicActorImpl::RequestBalancer(const TActorContext& ctx) {
635631
GotLocation = true;
636632
}
637633

638-
if (Settings.Mode == TDescribeTopicActorSettings::EMode::DescribeConsumer && Settings.RequireStats) {
634+
if (Settings.Mode == TDescribeTopicActorSettings::EMode::DescribeConsumer && Settings.RequireStats) {
639635
if (!GotReadSessions) {
640636
RequestReadSessionsInfo(ctx);
641637
}
@@ -671,7 +667,7 @@ void TDescribeTopicActorImpl::RequestPartitionsLocation(const TActorContext& ctx
671667
return RaiseError(
672668
TStringBuilder() << "No partition " << Settings.Partitions[0] << " in topic",
673669
Ydb::PersQueue::ErrorCode::BAD_REQUEST, Ydb::StatusIds::BAD_REQUEST, ctx
674-
);
670+
);
675671
}
676672
auto res = partIds.insert(p);
677673
if (res.second) {
@@ -705,7 +701,7 @@ void TDescribeTopicActorImpl::Handle(NKikimr::TEvPersQueue::TEvStatusResponse::T
705701

706702
auto& record = ev->Get()->Record;
707703
bool doRestart = (record.PartResultSize() == 0);
708-
704+
709705
for (auto& partResult : record.GetPartResult()) {
710706
if (partResult.GetStatus() == NKikimrPQ::TStatusResponse::STATUS_INITIALIZING ||
711707
partResult.GetStatus() == NKikimrPQ::TStatusResponse::STATUS_UNKNOWN) {
@@ -917,7 +913,7 @@ bool TDescribeTopicActor::ApplyResponse(
917913
}
918914
return true;
919915
}
920-
916+
921917

922918

923919
void TDescribeTopicActor::Reply(const TActorContext& ctx) {
@@ -1030,7 +1026,7 @@ bool TDescribeConsumerActor::ApplyResponse(
10301026
}
10311027
return true;
10321028
}
1033-
1029+
10341030

10351031
bool FillConsumerProto(Ydb::Topic::Consumer *rr, const NKikimrPQ::TPQTabletConfig& config, ui32 i,
10361032
const NActors::TActorContext& ctx, Ydb::StatusIds::StatusCode& status, TString& error)
@@ -1272,11 +1268,11 @@ bool TDescribeTopicActorImpl::ProcessTablets(
12721268
Tablets[pi.GetTabletId()].Partitions.push_back(pi.GetPartitionId());
12731269
Tablets[pi.GetTabletId()].TabletId = pi.GetTabletId();
12741270
}
1275-
1271+
12761272
for (auto& pair : Tablets) {
12771273
RequestTablet(pair.second, ctx);
12781274
}
1279-
1275+
12801276
if (RequestsInfly == 0) {
12811277
Reply(ctx);
12821278
return false;
@@ -1332,7 +1328,7 @@ void TDescribePartitionActor::Bootstrap(const NActors::TActorContext& ctx)
13321328

13331329
void TDescribePartitionActor::StateWork(TAutoPtr<IEventHandle>& ev) {
13341330
switch (ev->GetTypeRewrite()) {
1335-
default:
1331+
default:
13361332
if (!TDescribeTopicActorImpl::StateWork(ev, ActorContext())) {
13371333
TBase::StateWork(ev);
13381334
};
@@ -1359,12 +1355,12 @@ void TDescribePartitionActor::ApplyResponse(TTabletInfo&, NKikimr::TEvPersQueue:
13591355

13601356
void TDescribePartitionActor::ApplyResponse(TTabletInfo& tabletInfo, NKikimr::TEvPersQueue::TEvStatusResponse::TPtr& ev, const TActorContext&) {
13611357
auto* partResult = Result.mutable_partition();
1362-
1358+
13631359
const auto& record = ev->Get()->Record;
13641360
for (auto partData : record.GetPartResult()) {
13651361
if ((ui32)partData.GetPartition() != Settings.Partitions[0])
13661362
continue;
1367-
1363+
13681364
Y_ABORT_UNLESS((ui32)(partData.GetPartition()) == Settings.Partitions[0]);
13691365
partResult->set_partition_id(partData.GetPartition());
13701366
partResult->set_active(true);
@@ -1411,7 +1407,7 @@ void TDescribePartitionActor::Reply(const TActorContext& ctx) {
14111407

14121408
using namespace NIcNodeCache;
14131409

1414-
TPartitionsLocationActor::TPartitionsLocationActor(const TGetPartitionsLocationRequest& request, const TActorId& requester)
1410+
TPartitionsLocationActor::TPartitionsLocationActor(const TGetPartitionsLocationRequest& request, const TActorId& requester)
14151411
: TBase(request, requester)
14161412
, TDescribeTopicActorImpl(TDescribeTopicActorSettings::GetPartitionsLocation(request.PartitionIds))
14171413
{
@@ -1429,7 +1425,7 @@ void TPartitionsLocationActor::Bootstrap(const NActors::TActorContext&)
14291425
void TPartitionsLocationActor::StateWork(TAutoPtr<IEventHandle>& ev) {
14301426
switch (ev->GetTypeRewrite()) {
14311427
hFunc(TEvICNodesInfoCache::TEvGetAllNodesInfoResponse, Handle);
1432-
default:
1428+
default:
14331429
if (!TDescribeTopicActorImpl::StateWork(ev, ActorContext())) {
14341430
TBase::StateWork(ev);
14351431
};

‎ydb/services/persqueue_v1/persqueue_compat_ut.cpp

+4-2
Original file line numberDiff line numberDiff line change
@@ -174,13 +174,15 @@ Y_UNIT_TEST_SUITE(TPQCompatTest) {
174174
// Local topic with client write disabled
175175
testServer.CreateTopic("/Root/LbCommunal/some-topic", "account", false, true);
176176
// Non-local topic with client write enabled
177-
testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc2", "account", true, true);
178177
testServer.CreateTopic("/Root/LbCommunal/.some-topic/mirrored-from-dc2", "account", true, true);
178+
// this validation was relaxed
179+
testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc2", "account", true, false);
179180
// No account
180181
testServer.CreateTopic("/Root/LbCommunal/some-topic", "", true, true);
181182
// Mirrored-from local
182183
testServer.CreateTopic("/Root/LbCommunal/.some-topic/mirrored-from-dc1", "account", false, true);
183-
testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc1", "account", false, true);
184+
// this validation was relaxed
185+
testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc1", "account", false, false);
184186
// Bad mirrored names
185187
testServer.CreateTopic("/Root/LbCommunal/.some-topic/some-topic", "account", false, true);
186188
// Mirrored-from non-existing

0 commit comments

Comments
 (0)