Closed LeonidVas closed 2 years ago
The task requires research to obtain information about its feasibility and complexity.
I don't see too big problems with the identify handling (we will need an additional temporary space + handle it on ro->rw switch), but this approach would complicate a disconnect behavior too much (omit the complexity of implementation of different behaviors).
Suppose we have a master and a replica instance. An user of the queue
already identify on it. Such situations are possible:
We can define behavior for these cases, but I don't see an obvious one. In the same time the current implementation is simple for these cases (just wait for queue.cfg.ttr
).
In any case, the user will always have an additional code for routing and a master discovery. I don't see much benefit from the fact that the user will can to omit a call queue.identify()
in addition to the code.
So my opinion:
This is not a very useful feature that logically competes with queue.cfg.ttr
:
https://github.com/tarantool/queue/blob/2dd59489fd52186a918e37845c8aa219240ea90a/README.md?plain=1#L460-L461
It will not just overcomplicate our code and also make it difficult for the user (and developers) to understand the module behavior.
In a nutshell:
It will be a counterintuitive behavior of disconnect in case of multiple sessions.
We discussed in with Oleg in a chat. The short summary: I don't mind to throw it out at least until we'll see another case, where it is needed (except a binding to the queue on top of a connection pool with master discovery). The binding may be implemented without this feature.
Александр Туренко (12.08.2022 16:21): Я правильно понимаю, что проблема будет, если клиент сменил uuid на одном соединении, но не сделал этого для остальных? |
Александр Туренко (12.08.2022 16:21): Точнее, на соединении к одному инстансу сменил, а на соединениях к остальным — нет. |
Олег Жуковец (12.08.2022 16:23): Да, на мастере отключились или сменили session uuid по какой-то причине, а на реплике мы всё ещё подключены. |
Александр Туренко (12.08.2022 16:23): Это выглядит как misuse. |
Александр Туренко (12.08.2022 16:24): Ну, т. е. PoolClient явно должен обновлять uuid везде, когда обновляет uuid. |
Александр Туренко (12.08.2022 16:24): Если мы подразумеваем, что его можно в любом статусе очереди выбрать. |
Александр Туренко (12.08.2022 16:27): С т. з. queue к нам приходит некто, кто поставил uuid, под которым хочет работать. Если этот некто решил, что uuid, поставленный где-то еще, сработает тут, то это странно. |
Александр Туренко (12.08.2022 16:28): А с т. з. PoolClient — он хранит текущий uuid и ставит его для новых соединений. Если uuid меняется, то меняет везде. |
Александр Туренко (12.08.2022 16:29): В таких предположениях у меня картинка складывается. |
Олег Жуковец (12.08.2022 17:26): > С т. з. queue к нам приходит некто, кто поставил uuid, под которым хочет работать. Если этот некто решил, что uuid, поставленный где-то еще, сработает тут, то это странно. На мастере почему-то отключилась-разорвалась сессия, но на реплике она есть. Отменять задачу или ставить ей TTL в этом случае сходу выглядит странно - сессия то жива, пусть и на реплике. > А с т. з. PoolClient — он хранит текущий uuid и ставит его для новых соединений. Если uuid меняется, то меняет везде. А если рвётся соединение с мастером, но есть куча queue.identify(uuid) на других инстансах? Будет неожиданно получить возвращение задачи по TTL после того как удастся соединиться с новым или старым мастером. Хотя в этом плане я не особо спорю, определить какое-то поведение действительно можно. Но это всё рассуждение о поведении на уровне роутера-пула соединений-топологии. Т.е. на уровне queue закладываем некоторую логику роутера и нарушаем разделение ответственности. 1. Предполагаемое нами поведение может быть не очевидно для пользователя в крайних случаях. 2. Это только всё усложняет и ничего особо не даёт. Всё равно пользователю надо следить за соединениями, надо искать активного мастера, надо в него слать запросы - ну будет перед всем этим вполне логичный вызов `queue.identify()`. Сейчас же для пользователя всё просто: есть сессия на rw-инстансе, и есть `queue.cfg.ttr` на каждом инстансе. |
Александр Туренко (12.08.2022 17:31): Кажется, я улавливаю мысль. Если у нас отрубается единственное авторизованное соединение, то выглядит логично по queue.cfg.ttr вернуть задачи в ready. Если их несколько, то это уже контринтуитивно. Я верно понимаю мысль? |
Олег Жуковец (12.08.2022 17:32): Да |
Олег Жуковец (12.08.2022 17:36): И если бы это давало какие-то очевидные преимущества, то и отлично. А так я не вижу ради чего дополнительная сложность (даже не в реализации, а в самом поведении). |
Александр Туренко (12.08.2022 17:43): Мысль изначально шла от того, что в реализации биндинга к queue поверх connection pool сейчас потребуется коллбек на смену мастера (и гарантия, что он будет исполнен до любого запроса на этом мастере). Мне показалось, что альтернатива — коллбек на подключение — будет проще. Плюс еще было опасение по поводу быстро скачущего мастера (когда мы приходим с identify уже на RO), но, думаю, это можно продумать и корректно захендлить. |
Олег Жуковец (12.08.2022 18:02): На мой вкус всё же это делать на пуле соединений будет покрасивее. Кроме как потенциальных сложностей с реализацией (но они и в queue могут быть) никаких проблем в логике происходящего не вижу. |
Александр Туренко (12.08.2022 18:09): Я не возражаю (тем более, что новая функциональность будет не во всех релизах queue). Но я бы не называл это реализацией логики для пулла в queue, тут все-таки просто возможность авторизоваться заранее. |
So, after talking with Oleg, I agree that adding the ability to have several connections in replicaset only complicates the logic and does not add convenience to the client.
Now in case of a master switching, we must to re-identify on the new master. Seems like it would be convenient to connect to every instance in a replicaset, identify, and not have to think about identifying in case of master changing. Looks like it might be possible to implement this by using temporary spaces.