HH-124106 add sync cached based on consul blocking queries #3

nicholasgribanov · 2021-02-17T15:59:47Z

No description provided.

dzharikhin · 2021-02-17T16:14:36Z

consul/base.py

+        self.watch_seconds = watch_seconds
+        self.index = None
+        self._running = True
+        self._cache_process = threading.Thread(target=self._update_cache, daemon=True)


_cache_process -> _cache_thread

bokshitsky · 2021-02-17T16:34:11Z

consul/base.py

+    def stop(self):
+        self._running = False
+
+    def add_listener(self, callback, trigger_current=False):


Возможно, правильнее чтобы callback не только value принимал но и key

А зачем, кстати, там в cache лежит dict всегда из 1-го элемента?

Прав, для текущих кешей это не нужно, но кажется, что при расширении могут быть разные варианты наполнения кеша с ключами. И да, тогда имеет смысл key тоже передавать в callback.

bokshitsky · 2021-02-17T16:37:26Z

consul/base.py

+                'wait': self.watch_seconds
+            }
+            log.debug(f'Param for health query: {params}')
+            self.index, values = self.health_client.service(**params)


long_pooling и блокировка на строчк
self.index, values = self.health_client.service(**params) происходит?

Судя по тому, что мы в ответ получаем сразу значения, то вызов блокирующий.
А ошибки тут обработать не нужно? Они могут быть?

Ошибки быть могут. Мы допускаем только 404, в случае других ошибок нужно выбрасывать исключение. Вот тут описаны ошибки, которые могу возникнуть

python-consul2/consul/base.py

Line 208 in b085726

def _status(klass, response, allow_404=True):

Тогда обращаю внимание, что этот отдельный тред в случае эксепшена просто умрет )

А вообще прав, я не подумал. Тред умрет, приложение будет дальше работать. Оберну в трай-кэтч, чтобы пока приложение живо поток пытался получить данные из консула

bokshitsky · 2021-02-17T16:39:52Z

consul/base.py

+    def __init__(self, watch_seconds):
+        self.cache = dict()
+        self.callbacks = []
+        self.watch_seconds = watch_seconds


для watch_seconds как параметра этот Cache-объекта имя бы какое-нибудь другое дать, просто щас непотяно что оно значит.
По факту же это сколько максимально по времени держать long-pooling соединение, в случае если ничего не изменится в консуле за это время?

Тут все так, это дань унификации. У нас в клиенте джавовом и конфигах он так же называется. Мне кажется лучше оставить так, чтобы разъездов меньше было. Если переименовывать, то по хорошему и в конфигах и в джавовом клиенте. Что думаешь?

думаю, надо оставить

bokshitsky · 2021-02-17T16:43:51Z

consul/base.py

+            self.index, values = self.kv_client.get(**params)
+            old_cache = self.cache
+            self.cache = {self.path: values}
+            if self.callbacks and self._running:


это на вкус канеш, но я бы инвертировал логику проверок с ранним выхходом на continue
чтобы пирамидку чутка разгрузить

sintell · 2021-02-17T17:02:19Z

consul/base.py

        return {'passing': passing, 'warning': warning}


+class ConsulCacheBase(object):


не надо от object наследоваться

sintell · 2021-02-17T17:05:25Z

consul/base.py

+        self.watch_seconds = watch_seconds
+        self.index = None
+        self._running = True
+        self._cache_process = threading.Thread(target=self._update_cache, daemon=True)


Я бы еще какой-нибудь понятный name задал бы, чтобы в трейсбеках можно было бы следить и для метрик может пригодиться

А есть конвенция как имена тредам давать в питоне? snake-case, camel-case?

не, нет какой-то особой, по-умолчанию они называются как Thread-N, где N это small int

sintell · 2021-02-17T17:12:56Z

consul/base.py

+            for key, value in self.cache:
+                callback(value)
+
+    def _update_cache(self):


Если не предполагается, что этот класс кто-то будет использовать напрямую, то я бы его объявил бы как

class ConsulCacheBase(metaclass=ABCMeta): ...

А на этот метод повесил бы
@abstractmethod

sintell · 2021-02-17T17:15:12Z

consul/base.py

+    """
+
+    def __init__(self, health_client, watch_seconds, service, passing):
+        ConsulCacheBase.__init__(self, watch_seconds)


лучше всегда использовать super().__init__. Тут это не критично, но если вдруг базщовый класс будет использовать множественное наследование то тут все развалится

sintell · 2021-02-17T17:16:16Z

consul/base.py

+    Consul health service cache
+    """
+
+    def __init__(self, health_client, watch_seconds, service, passing):


Укажи, пожалуйста типы аргументов, тут они совсем не очевидны

И, возможно стоит и значение некоторых пояснить, в докстринге, например - что такое passing

sintell · 2021-02-17T17:28:20Z

consul/base.py

+                'wait': self.watch_seconds
+            }
+            log.debug(f'Param for health query: {params}')
+            self.index, values = self.health_client.service(**params)


Судя по тому, что мы в ответ получаем сразу значения, то вызов блокирующий.
А ошибки тут обработать не нужно? Они могут быть?

sintell · 2021-02-17T17:32:03Z

consul/base.py

+            self.index, values = self.health_client.service(**params)
+            self.cache = {self.service: values}
+            old_cache = self.cache
+            if self.callbacks and self._running:


Нет смысла тут повторно проверять на is_running, потому что до этого момента никто не успеет этот флаг поменять.

могут по идее, тут ведь отдельный тред

Да, я чот плохо подумал, в теории GIL гарантирует эксклюзивный доступ, но тут будет блокировка на моменте self.health_client.service и вот тут может другой поток запуститься

sintell · 2021-02-17T17:35:17Z

consul/base.py

+            old_cache = self.cache
+            if self.callbacks and self._running:
+                for key, old_value in old_cache.items():
+                    new_value = self.cache.get(key, None)


А в чем логика callback(None)?

Ничего не получили из KV хранилища консула, должны в сервисе задать значение по умолчанию

sintell · 2021-02-17T17:35:55Z

consul/base.py

+    def __init__(self, kv_client, watch_seconds, path, total_timeout, consistency_mode):
+        ConsulCacheBase.__init__(self, watch_seconds)
+        self.total_timeout = total_timeout
+        self.consistency_mode = consistency_mode


Выглядит так, как будто это должен быть Enum

sintell · 2021-02-17T17:37:59Z

consul/base.py

+            if self.callbacks and self._running:
+                for key, new_value in self.cache.items():
+                    old_value = old_cache.get(key, None)
+                    if old_value != new_value:


Значения каких типов тут могут быть?

sintell · 2021-02-17T17:39:42Z

consul/base.py

    pass


+class ConsulCacheException(Exception):


Не нашел использований, заче эта ошибка?

sintell · 2021-02-17T17:42:18Z

consul/base.py

+        self.callbacks.append(callback)
+        log.debug(f'Registered callback: {self.callbacks}')
+        if trigger_current:
+            for key, value in self.cache:


Так не сработает, надо in self.cache.items()

dzharikhin · 2021-02-17T19:03:45Z

consul/base.py

+        self.consistency_mode = consistency_mode
+        self.path = path
+        self.kv_client = kv_client
+        self.cache = {self.path: kv_client.get(path)[1]}


вот тут давай-ка заложим-ка кастомный таймаут?
отхожим местом чую - понадобится он

dzharikhin · 2021-02-19T08:08:18Z

consul/base.py

+    def __init__(self, watch_seconds):
+        self.cache = dict()
+        self.callbacks = []
+        self.watch_seconds = watch_seconds


думаю, надо оставить

dzharikhin · 2021-02-19T08:18:07Z

consul/base.py

+                 kv_client: 'Consul.KV',
+                 watch_seconds: str,
+                 path: str,
+                 total_timeout: int,


cache_initial_warmup_timeout? и наверное этот параметр необязательный. если меня общий таймаут устраивает, то че бы нет

Имеешь ввиду еще один параметр добавить, чтобы тут его использовать при инициализации кеша ?
self.cache = {self.path: kv_client.get(key=path, total_timeout=total_timeout)[1]}

sintell · 2021-02-19T16:08:25Z

consul/base.py

+    calls to wait for changes since this query was last run.
+    """
+
+    def __init__(self, watch_seconds: str):


А watch_seconds точно должен быть str? Выглядит несколько нелогичным

Да, это консула. Он в таком виде : 10s, 10m, 10ms передается

sintell · 2021-02-19T16:08:44Z

consul/base.py

+    """
+
+    def __init__(self,
+                 health_client: 'Consul.Health',


А нельзя сюда просто заимпортить этот тип?

А как его "заимпортить"?

Что такое health_client, откуда он возьмётся?

sintell

Ещё пара моментов

sintell · 2021-02-24T07:27:47Z

consul/base.py

+                 watch_seconds: str,
+                 path: str,
+                 total_timeout: int,
+                 consistency_mode: 'ConsistencyMode'):


Тут можно тип без кавычек указать, он же в этом же файле, выше, определён

sintell · 2021-02-24T07:29:26Z

consul/base.py

+                            log.debug(f'Value was changed for key={key}. old: {old_value} new: {new_value}')
+                            for callback in self.callbacks:
+                                callback(key, new_value)
+            except Exception as e:


Использовать дженерный класс Exception считается плохой практикой, потому что:

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

постороннему разработчику не очевидно какие именно ошибки тут могут возникнуть

Тут точно нельзя указать конкретные ошибки, которые мы ждём?

Уточнил тип

dzharikhin reviewed Feb 17, 2021

View reviewed changes

dzharikhin approved these changes Feb 17, 2021

View reviewed changes

bokshitsky reviewed Feb 17, 2021

View reviewed changes

sintell requested changes Feb 17, 2021

View reviewed changes

sintell reviewed Feb 17, 2021

View reviewed changes

dzharikhin reviewed Feb 17, 2021

View reviewed changes

dzharikhin reviewed Feb 19, 2021

View reviewed changes

sintell reviewed Feb 19, 2021

View reviewed changes

sintell reviewed Feb 24, 2021

View reviewed changes

sintell approved these changes Feb 24, 2021

View reviewed changes

HH-124106 add sync cached based on consul blocking queries

c9b0123

nicholasgribanov force-pushed the HH-124106 branch from 2455fd5 to c9b0123 Compare February 24, 2021 09:40

nicholasgribanov merged commit 56e03f0 into master Feb 24, 2021

hh-infrastructure-bot deleted the HH-124106 branch August 1, 2025 22:55

		return {'passing': passing, 'warning': warning}


		class ConsulCacheBase(object):

HH-124106 add sync cached based on consul blocking queries #3

HH-124106 add sync cached based on consul blocking queries #3

Uh oh!

Conversation

nicholasgribanov commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sintell Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sintell Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sintell Feb 17, 2021 •

edited

Loading

sintell Feb 17, 2021 •

edited

Loading