LOGBROKER-8840_create_topics_kafka_endpoint #747
Conversation
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 95346b4.
🔴 linux-x86_64-release-asan: some tests FAILED for commit 95346b4.
|
| static const TString RETENTION_MS_CONFIG_NAME = "retention.ms"; | ||
| static const TString RETENTION_BYTES_CONFIG_NAME = "retention.bytes"; | ||
|
|
||
| class TKafkaCreateTopicRequest : public NKikimr::NGRpcService::IRequestOpCtx { |
There was a problem hiding this comment.
Возможно это надо вынести куда-нибудь выше. Вроде никак не связано с созданием , и можно будет переиспользовать в других акторах.
There was a problem hiding this comment.
Ага, это 100% ещё будет нужно в AlterConfigs.
Я добавил файл kafka_constants.h и вынес туда
| for (auto& topic : Message->Topics) { | ||
| auto& topicName = topic.Name.value(); | ||
|
|
||
| if (DuplicateTopicNames.contains(topicName)) { |
There was a problem hiding this comment.
Получается, что если топик указан два раза, то мы не попытаемся его даже создать. Так и задумывалось?
There was a problem hiding this comment.
Ага, если топик указали 2 раза - непонятно, какую из 2х конфигураций брать.
И ещё такая ситуация может возникнуть, если, например, не поменяли название топика при копипасте, поэтому опасно молча такое проглатывать
| std::optional<ui64> retentionMs; | ||
| std::optional<ui64> retentionBytes; | ||
|
|
||
| auto parseRetention = [this, topic]( |
There was a problem hiding this comment.
А почему это лямбда, а не функция?
There was a problem hiding this comment.
Мне больше нравится объявлять небольшие функции прямо там, где они используются. Так не принято? Если так, давай вынесу в метод
| TCreateTopicsResponseData::TPtr response = std::make_shared<TCreateTopicsResponseData>(); | ||
|
|
||
| for (auto& requestTopic : Message->Topics) { | ||
| auto topicName = requestTopic.Name.value(); |
There was a problem hiding this comment.
А это точно достаточная валидация? Или это для доработок потом?
(например, проверить правильный retaintion или существование топика)
There was a problem hiding this comment.
Да, обсуждали с Лёшей, решили, что вообще не нужно запариваться - отвечать ОК на вообще всё
Если будет запрос на эту фичу - тогда и сделаем полноценную валидацию
| response->TopicPath = TopicPath; | ||
| response->Message = message; | ||
| Send(Requester, response.Release()); | ||
| Send(SelfId(), new TEvents::TEvPoisonPill()); |
There was a problem hiding this comment.
fyi TEvPoisonPill was deprecated. Use TEvPoison
siarheivesialou
force-pushed
the
LOGBROKER-8840_create_topics_kafka_endpoint
branch
from
554759a to
95346b4
Compare
siarheivesialou
deleted the
LOGBROKER-8840_create_topics_kafka_endpoint
branch
No description provided.