feat etcd: Init etcd client #837

eskedesu · 2025-01-20T08:37:57Z

Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.

cmake/install/userver-etcd-config.cmake

segoon · 2025-01-20T08:43:20Z

etcd/include/userver/storages/etcd/component.hpp

+#include <userver/components/component_config.hpp>
+#include <userver/components/component_context.hpp>
+#include <userver/storages/etcd/client.hpp>
+


во все публичные хедеры нужно добавить документацию
в заголовок файла
перед каждым классом
перед каждым методом

segoon · 2025-01-20T08:44:10Z

etcd/include/userver/storages/etcd/client.hpp

+
+namespace storages::etcd {
+
+class Client final {


может лучше не final, а абстрактный класс? чтобы можно было замокать для тестирования клиента конфигов с использованием etcd

segoon · 2025-01-20T08:44:22Z

etcd/include/userver/storages/etcd/settings.hpp

+
+struct ClientSettings final {
+    std::vector<std::string> endpoints;
+    std::uint32_t retries;


segoon · 2025-01-20T08:45:29Z

etcd/src/storages/etcd/client.cpp

+}
+
+std::string BuildPutData(const std::string& key, const std::string& value) {
+    formats::json::StringBuilder sb;


тут скорость не так важна
можно и просто из inline.hpp взять MakeObject("key", x, "value", y)

segoon · 2025-01-20T08:50:15Z

etcd/src/storages/etcd/component.cpp

+        minimum: 1
+    request_timeout_ms:
+        type: integer
+        description: Number of miliseconds to timeout request


between request attempts

segoon · 2025-01-20T08:51:55Z

etcd/include/userver/storages/etcd/client.hpp

+    Client(clients::http::Client& http_client, ClientSettings settings);
+    void Put(const std::string& key, const std::string& value);
+    [[nodiscard]] std::vector<std::string> Range(const std::string& key);
+    void DeleteRange(const std::string& key);


для клиента конфигов нам нужны:

получение значения по ключу

получение всех значений (это range&)

вотч на все значения
возможно не все, а только те, что начинаются с определенного префикса

segoon · 2025-01-20T08:52:29Z

etcd/include/userver/storages/etcd/client.hpp

+    Client(clients::http::Client& http_client, ClientSettings settings);
+    void Put(const std::string& key, const std::string& value);
+    [[nodiscard]] std::vector<std::string> Range(const std::string& key);
+    void DeleteRange(const std::string& key);


для клиента конфигов очень хочется иметь юнит-тест

segoon · 2025-01-20T08:52:56Z

etcd/include/userver/storages/etcd/client.hpp

+    Client(clients::http::Client& http_client, ClientSettings settings);
+    void Put(const std::string& key, const std::string& value);
+    [[nodiscard]] std::vector<std::string> Range(const std::string& key);
+    void DeleteRange(const std::string& key);


для клиента конфигов очень хочется иметь testsuite-тест, который получает значения конфига из мока

segoon · 2025-01-20T08:54:07Z

etcd/src/storages/etcd/client.cpp

+    return sb.GetString();
+}
+
+bool ShouldRetry(std::shared_ptr<clients::http::Response> response) {


тут что-то должно быть? для того, чтобы фолбечиться на другой бекенд при отказе одно

segoon · 2025-02-03T02:40:09Z

etcd/include/userver/storages/etcd/client.hpp

@@ -0,0 +1,63 @@
+#pragma once


без storages/

segoon · 2025-02-03T02:40:16Z

etcd/include/userver/storages/etcd/client.hpp

+
+USERVER_NAMESPACE_BEGIN
+
+namespace storages::etcd {


без storages

segoon · 2025-02-03T02:41:08Z

etcd/include/userver/storages/etcd/component.hpp

+
+class Component final : public components::ComponentBase {
+public:
+    static constexpr std::string_view kName = "etcd";


etcd-client

Поправил

segoon · 2025-02-03T02:41:17Z

etcd/include/userver/storages/etcd/component.hpp

+
+    Component(const components::ComponentConfig&, const components::ComponentContext&);
+
+    ~Component() = default;


не нужно

segoon · 2025-02-03T02:42:10Z

etcd/include/userver/storages/etcd/watch_listener.hpp

+
+namespace storages::etcd {
+
+struct KVEvent final {


KeyValueEvent, не нужно сокращать такие короткие имена

Поправил

segoon · 2025-02-03T02:45:26Z

etcd/src/storages/etcd/client.cpp

+        "watch task",
+        [stream_response = std::move(stream_response), produser = queue->GetProducer()] mutable {
+            std::string body_part;
+            const auto deadline = engine::Deadline::FromDuration(std::chrono::seconds{100'000'000});


можно просто не указывать дедлайн

segoon · 2025-02-03T02:45:38Z

etcd/src/storages/etcd/client.cpp

+                const auto watch_response = formats::json::FromString(body_part);
+                LOG_ERROR() << watch_response;
+                if (!watch_response["result"].HasMember("events")) {
+                    LOG_INFO() << "No events in watch part response, skipping";


segoon · 2025-02-03T02:46:13Z

etcd/src/storages/etcd/client.cpp

+                    }
+                    const auto key = crypto::base64::Base64Decode(event["kv"]["key"].As<std::string>());
+                    const auto value = crypto::base64::Base64Decode(event["kv"]["value"].As<std::string>());
+                    const auto version = std::stoi(event["kv"]["version"].As<std::string>());


парсинг евента нужно точно через chaotic делать

segoon · 2025-02-03T02:46:35Z

etcd/src/storages/etcd/client.cpp

+                .CreateRequest()
+                .post(url_builder(endpoint), data)
+                .retry(settings_.retries)
+                .timeout(1'000'000'000)


в статический конфиг

segoon · 2025-02-03T02:48:20Z

etcd/include/userver/storages/etcd/client.hpp

+#pragma once
+
+#include <chrono>
+#include <cstdint>


давай сразу клиента конфига сделаешь до полноценного watch event'а - так будет проще понять, какой API требуется и что еще необходимо сделать

segoon · 2025-02-16T19:54:03Z

etcd/src/etcd/client_impl.cpp

+                                   .timeout(1'000'000'000)
+                                   .async_perform_stream_body(queue);
+        if (!ShouldRetry(stream_response.StatusCode())) {
+            CheckRequestStatusCode(stream_response.StatusCode());


stream_response.RaiseForError()?

Кажется, для StreamedResponse нет такого метода

segoon · 2025-02-16T19:55:10Z

etcd/src/etcd/client_impl.cpp

+        auto stream_response = http_client_.CreateRequest()
+                                   .post(url_builder(endpoint), data)
+                                   .retry(settings_.attempts)
+                                   .timeout(1'000'000'000)


какая будет защита от залипшего соединения?
в целом, можно отдельно про это подумать

segoon · 2025-02-16T19:55:42Z

etcd/src/etcd/client_impl.cpp

+            return stream_response;
+        }
+    }
+    throw EtcdError("Failed to get Ok response from etcd");


а ошибка какая? или хотя бы статус код(ы) какой?

segoon · 2025-02-16T19:55:56Z

etcd/src/etcd/client_impl.cpp

+            return response;
+        }
+    }
+    throw EtcdError("Failed to get Ok response from etcd");


аналогично, мало информации об ошибке

segoon · 2025-02-16T19:56:19Z

etcd/src/etcd/client_impl.hpp

+    );
+
+    clients::http::Client& http_client_;
+    engine::Mutex watch_queues_lock_;


concurrent::Variable

segoon · 2025-02-16T19:59:58Z

etcd/src/etcd/client_impl.hpp

+
+class ClientImpl : public Client {
+public:
+    ~ClientImpl() override = default;


override = default выглядит странно

segoon · 2025-02-16T20:00:07Z

etcd/src/etcd/client_impl.hpp

+public:
+    ~ClientImpl() override = default;
+    ClientImpl(clients::http::Client& http_client, ClientSettings settings);
+    void Put(const std::string& key, const std::string& value) override;


разделяй методы пустой строкой

segoon · 2025-02-16T20:01:12Z

etcd/src/etcd/client_impl.cpp

+                if (!event.HasMember("kv")) {
+                    continue;
+                }
+                if (!produser.PushNoblock(event["kv"].As<KeyValueEvent>())) {


почему Noblock?

segoon · 2025-02-16T20:01:50Z

etcd/src/etcd/client_impl.cpp

+    utils::Async("watch task", [stream_response = std::move(stream_response), produser = queue->GetProducer()] mutable {
+        std::string body_part;
+        const auto deadline = engine::Deadline::FromDuration(std::chrono::seconds{100'000'000});
+        while (stream_response.ReadChunk(body_part, deadline)) {


давай передавать распаршенное тело
лучше разделить парсинг и интерпретацию ответов

segoon · 2025-02-16T20:02:04Z

etcd/src/etcd/client_impl.cpp

+    watch_queues_.push_back(queue);
+    watch_queues_lock_.unlock();
+
+    utils::Async("watch task", [stream_response = std::move(stream_response), produser = queue->GetProducer()] mutable {


Поправил

eskedesu added 4 commits January 17, 2025 01:09

init etcd client

145a211

minor fixes

b184754

iteration

bf56139

rename client

3c1e377

segoon reviewed Jan 20, 2025

View reviewed changes

eskedesu added 2 commits February 2, 2025 01:28

iteration

fc3f6e7

iteration

a1119f6

segoon reviewed Feb 3, 2025

View reviewed changes

iteration

330ef41

segoon reviewed Feb 16, 2025

View reviewed changes

iteration

dfcee7d


		Component(const components::ComponentConfig&, const components::ComponentContext&);

		~Component() = default;


		USERVER_NAMESPACE_BEGIN

		namespace storages::etcd {

feat etcd: Init etcd client #837

Are you sure you want to change the base?

feat etcd: Init etcd client #837

Conversation

eskedesu commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment