Refactor mirrored topic names

3 files changed, 33 insertions, 32 deletions

diff --git a/ydb/library/persqueue/topic_parser/topic_parser.cpp b/ydb/library/persqueue/topic_parser/topic_parser.cpp
index f0f80e93469..72fa54f93a9 100644
--- a/ydb/library/persqueue/topic_parser/topic_parser.cpp
+++ b/ydb/library/persqueue/topic_parser/topic_parser.cpp
@@ -54,7 +54,6 @@ TString NormalizeFullPath(const TString& fullPath) {
     }
 }
 
-
 TString GetFullTopicPath(const NActors::TActorContext& ctx, const TMaybe<TString>& database, const TString& topicPath) {
     if (NKikimr::AppData(ctx)->PQConfig.GetTopicsAreFirstClassCitizen()) {
         return FullPath(database, topicPath);
@@ -149,8 +148,11 @@ TDiscoveryConverter::TDiscoveryConverter(bool firstClass,
             res_ = pathBuf.SkipPrefix("/");
             TStringBuf acc, rest;
             res_ = pathBuf.TrySplit("/", acc, rest);
-            Y_VERIFY(res_);
+            if (res_) {
             Database = NKikimr::JoinPath({TString(dbRoot), TString(acc)});
+            } else {
+                Database = TString(dbRoot);
+            }
         }
     }
     if (!Database.Defined()) {
@@ -293,32 +295,21 @@ void TDiscoveryConverter::BuildFromFederationPath(const TString& rootPrefix) {
 }
 
 bool TDiscoveryConverter::TryParseModernMirroredPath(TStringBuf path) {
-    if (!path.Contains(".") || !path.Contains("/mirrored-from-")) {
+    if (!path.Contains("-mirrored-from-")) {
+        CHECK_SET_VALID(!path.Contains("mirrored-from"), "Federation topics cannot contain 'mirrored-from' in name unless this is a mirrored topic", return false); 
         return false;
     }
     TStringBuf fst, snd;
-    auto res = path.TryRSplit("/", fst, snd);
-    CHECK_SET_VALID(res, "Malformed mirrored topic path - expected to end with '/mirrored-from-<cluster>'", return false);
-    CHECK_SET_VALID(!snd.empty(), "Malformed mirrored topic path - expected to end with '/mirrored-from-<cluster>",
+    auto res = path.TryRSplit("-mirrored-from-", fst, snd);
+    CHECK_SET_VALID(res, "Malformed mirrored topic path - expected to end with '-mirrored-from-<cluster>'", return false);
+    CHECK_SET_VALID(!snd.empty(), "Malformed mirrored topic path - expected to end with valid cluster name",
                     return false);
-    res = snd.SkipPrefix("mirrored-from-");
-    CHECK_SET_VALID(res, "Malformed mirrored topic path - invalid '/mirrored-from-<cluster>; part" , return false);
     CHECK_SET_VALID(Dc.empty() || Dc == "unknown" || Dc == snd,
                     "Bad mirrored topic path - cluster in name mismatches with cluster provided",
                     return false);
     Dc = snd;
     FullModernName = path;
-    path = fst;
-    res = path.TryRSplit("/", fst, snd);
-    if (res) {
-        res = snd.SkipPrefix(".");
-        CHECK_SET_VALID(res, "Malformed mirrored topic path - topic name is expected to start with '.'", return false);
-        ModernName = NKikimr::JoinPath({TString(fst), TString(snd)});
-    } else {
-        res = path.SkipPrefix(".");
-        CHECK_SET_VALID(res, "Malformed mirrored topic path - topic name is expected to start with '.'", return false);
-        ModernName = path;
-    }
+    ModernName = fst;
     if (Account_.Defined()) {
         BuildFromShortModernName();
     }
@@ -330,14 +321,14 @@ void TDiscoveryConverter::ParseModernPath(const TStringBuf& path) {
     // So only convention supported is 'account/dir/topic' and account in path matches LB account;
     TStringBuilder pathAfterAccount;
 
-    // Path after account would contain 'dir/topic' OR dir/.topic/mirrored-from-..
+    // Path after account would contain 'dir/topic' OR dir/topic-mirrored-from-...
     if (!Dc.empty() && !LocalDc.empty() && Dc != LocalDc) {
         TStringBuf directories, topicName;
         auto res = path.TrySplit("/", directories, topicName);
         if (res) {
-            pathAfterAccount << directories << "/." << topicName << "/mirrored-from-" << Dc;
+            pathAfterAccount << directories << "/" << topicName << "-mirrored-from-" << Dc;
         } else {
-            pathAfterAccount << "." << path << "/mirrored-from-" << Dc;
+            pathAfterAccount << path << "-mirrored-from-" << Dc;
         }
     } else {
         pathAfterAccount << path;
@@ -473,7 +464,7 @@ void TDiscoveryConverter::BuildFromLegacyName(const TString& rootPrefix, bool fo
     Y_VERIFY(!Dc.empty());
     bool isMirrored = (!LocalDc.empty() && Dc != LocalDc);
     if (isMirrored) {
-        fullModernName << "." << topicName << "/mirrored-from-" << Dc;
+        fullModernName << topicName << "-mirrored-from-" << Dc;
     } else {
         fullModernName << topicName;
     }
diff --git a/ydb/library/persqueue/topic_parser/ut/topic_names_converter_ut.cpp b/ydb/library/persqueue/topic_parser/ut/topic_names_converter_ut.cpp
index 5762e03cd53..9caf5ee2f3e 100644
--- a/ydb/library/persqueue/topic_parser/ut/topic_names_converter_ut.cpp
+++ b/ydb/library/persqueue/topic_parser/ut/topic_names_converter_ut.cpp
@@ -229,15 +229,16 @@ Y_UNIT_TEST_SUITE(TopicNameConverterTest) {
             NKikimrPQ::TPQTabletConfig pqConfig;
             pqConfig.SetTopicName("rt3.dc2--account@path--topic");
             pqConfig.SetLocalDC(false);
-            pqConfig.SetTopicPath("/lb/account-database/path/.topic/mirrored-from-dc2");
+            pqConfig.SetTopicPath("/lb/account-database/path/topic-mirrored-from-dc2");
             pqConfig.SetFederationAccount("account");
             pqConfig.SetYdbDatabasePath("/lb/account-database");
 
             wrapper.SetConverter(pqConfig);
 
-            UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetPrimaryPath(), "/lb/account-database/path/.topic/mirrored-from-dc2");
+            UNIT_ASSERT_C(wrapper.TopicConverter->IsValid(), wrapper.TopicConverter->GetReason());
+            UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetPrimaryPath(), "/lb/account-database/path/topic-mirrored-from-dc2");
             UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetSecondaryPath(), "/Root/PQ/rt3.dc2--account@path--topic");
-            UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetModernName(), "path/.topic/mirrored-from-dc2");
+            UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetModernName(), "path/topic-mirrored-from-dc2");
             UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetClientsideName(), "rt3.dc2--account@path--topic");
             UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetFederationPath(), "account/path/topic");
             UNIT_ASSERT_VALUES_EQUAL(wrapper.TopicConverter->GetInternalName(), "rt3.dc2--account@path--topic");
@@ -327,14 +328,14 @@ Y_UNIT_TEST_SUITE(TopicNameConverterForCPTest) {
         UNIT_ASSERT_VALUES_EQUAL(converter->GetPrimaryPath(), "/LbCommunal/account/topic");
 
         converter = TTopicNameConverter::ForFederation(
-                "/Root/PQ", "", "mirrored-from-sas", "/LbCommunal/account/dir/.topic", "/LbCommunal/account", false, "", "account"
+                "/Root/PQ", "", "topic-mirrored-from-sas", "/LbCommunal/account/dir", "/LbCommunal/account", false, "", "account"
         );
         UNIT_ASSERT_C(converter->IsValid(), converter->GetReason());
         UNIT_ASSERT_VALUES_EQUAL(converter->GetAccount(), "account");
         UNIT_ASSERT_VALUES_EQUAL(converter->GetCluster(), "sas");
         UNIT_ASSERT_VALUES_EQUAL(converter->GetLegacyProducer(), "account@dir");
         UNIT_ASSERT_VALUES_EQUAL(converter->GetLegacyLogtype(), "topic");
-        UNIT_ASSERT_VALUES_EQUAL(converter->GetPrimaryPath(), "/LbCommunal/account/dir/.topic/mirrored-from-sas");
+        UNIT_ASSERT_VALUES_EQUAL(converter->GetPrimaryPath(), "/LbCommunal/account/dir/topic-mirrored-from-sas");
     }
 
     Y_UNIT_TEST(BadModernTopics) {
@@ -354,7 +355,12 @@ Y_UNIT_TEST_SUITE(TopicNameConverterForCPTest) {
         UNIT_ASSERT(!converter->IsValid());
 
         converter = TTopicNameConverter::ForFederation(
-                "", "", "mirrored-from-sas", "/LbCommunal/account/.topic", "/LbCommunal/account", true, "sas", "account"
+                "", "", "topic-mirrored-from-sas", "/LbCommunal/account", "/LbCommunal/account", true, "sas", "account"
+        );
+        UNIT_ASSERT(!converter->IsValid());
+
+        converter = TTopicNameConverter::ForFederation(
+                "", "", "topic-mirrored-from-", "/LbCommunal/account", "/LbCommunal/account", true, "sas", "account"
         );
         UNIT_ASSERT(!converter->IsValid());
 
diff --git a/ydb/services/persqueue_v1/persqueue_compat_ut.cpp b/ydb/services/persqueue_v1/persqueue_compat_ut.cpp
index 93e2bf4f1e4..b396f149c2f 100644
--- a/ydb/services/persqueue_v1/persqueue_compat_ut.cpp
+++ b/ydb/services/persqueue_v1/persqueue_compat_ut.cpp
@@ -39,7 +39,7 @@ public:
         );
 
         Server->AnnoyingClient->CreateTopicNoLegacy(
-                "/Root/LbCommunal/account/.topic2/mirrored-from-dc2", 1, true, false, {}, {}, "account"
+                "/Root/LbCommunal/account/topic2-mirrored-from-dc2", 1, true, false, {}, {}, "account"
         );
         Server->AnnoyingClient->CreateConsumer("test-consumer");
         InitPQLib();
@@ -148,6 +148,7 @@ Y_UNIT_TEST_SUITE(TPQCompatTest) {
     Y_UNIT_TEST(BadTopics) {
         TPQv1CompatTestBase testServer;
         testServer.CreateTopic("/Root/PQ/some-topic", "", true, true);
+        testServer.CreateTopic("/Root/PQ/some-topic-mirrored-from-dc2", "", false, true);
         testServer.CreateTopic("/Root/PQ/.some-topic/mirrored-from-dc2", "", false, true);
 
         //Bad dc
@@ -162,19 +163,22 @@ Y_UNIT_TEST_SUITE(TPQCompatTest) {
 
         // Local topic with client write disabled
         testServer.CreateTopic("/Root/LbCommunal/some-topic", "account", false, true);
-        // Non-local topic with client write disabled
+        // Non-local topic with client write enabled
+        testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc2", "account", true, true);
         testServer.CreateTopic("/Root/LbCommunal/.some-topic/mirrored-from-dc2", "account", true, true);
         // No account
         testServer.CreateTopic("/Root/LbCommunal/some-topic", "", true, true);
         // Mirrored-from local
         testServer.CreateTopic("/Root/LbCommunal/.some-topic/mirrored-from-dc1", "account", false, true);
+        testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc1", "account", false, true);
         // Bad mirrored names
         testServer.CreateTopic("/Root/LbCommunal/.some-topic/some-topic", "account", false, true);
         // Mirrored-from non-existing
         testServer.CreateTopic("/Root/LbCommunal/.some-topic/mirrored-from-dc777", "account", false, true);
+        testServer.CreateTopic("/Root/LbCommunal/some-topic-mirrored-from-dc777", "account", false, true);
 
         // Just to verify it even creates anything at all
-        testServer.CreateTopic("/Root/LbCommunal/.some-topic/mirrored-from-dc2", "account", false, false);
+        testServer.CreateTopic("/Root/LbCommunal/account/some-topic-mirrored-from-dc2", "account", false, false);
 
     }
 }