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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

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

Merged
nicholasgribanov merged 1 commit into
masterfrom
HH-124106
Feb 24, 2021
Merged

HH-124106 add sync cached based on consul blocking queries#3
nicholasgribanov merged 1 commit into
masterfrom
HH-124106

Conversation

@nicholasgribanov

Copy link
Copy Markdown

No description provided.

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_cache_process -> _cache_thread

Comment threadconsul/base.py
def stop(self):
self._running = False

def add_listener(self, callback, trigger_current=False):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def_status(klass, response, allow_404=True):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.kv_client.get(**params)
old_cache = self.cache
self.cache = {self.path: values}
if self.callbacks and self._running:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
return {'passing': passing, 'warning': warning}


class ConsulCacheBase(object):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.watch_seconds = watch_seconds
self.index = None
self._running = True
self._cache_process = threading.Thread(target=self._update_cache, daemon=True)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
for key, value in self.cache:
callback(value)

def _update_cache(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classConsulCacheBase(metaclass=ABCMeta):
...

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
Consul health service cache
"""

def __init__(self, health_client, watch_seconds, service, passing):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
'wait': self.watch_seconds
}
log.debug(f'Param for health query: {params}')
self.index, values = self.health_client.service(**params)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.index, values = self.health_client.service(**params)
self.cache = {self.service: values}
old_cache = self.cache
if self.callbacks and self._running:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
pass


class ConsulCacheException(Exception):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.callbacks.append(callback)
log.debug(f'Registered callback: {self.callbacks}')
if trigger_current:
for key, value in self.cache:

@sintellsintellFeb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
self.consistency_mode = consistency_mode
self.path = path
self.kv_client = kv_client
self.cache = {self.path: kv_client.get(path)[1]}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
def __init__(self, watch_seconds):
self.cache = dict()
self.callbacks = []
self.watch_seconds = watch_seconds

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
kv_client: 'Consul.KV',
watch_seconds: str,
path: str,
total_timeout: int,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py
calls to wait for changes since this query was last run.
"""

def __init__(self, watch_seconds: str):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
"""

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sintellsintell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadconsul/base.py Outdated
watch_seconds: str,
path: str,
total_timeout: int,
consistency_mode: 'ConsistencyMode'):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

есть!

Comment threadconsul/base.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уточнил тип

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nicholasgribanov@sintell@bokshitsky@dzharikhin