Feature seems useful for me as it makes connection management more robust and predictable.
I agree with Pavel, that we should print warning when heartbeat period is larger than idle timeout, but I see a problem here as idle timeout is configured on server and is not known to clients, while heartbeats configured on clients and their period is not known to the server. Maybe clients should pass this information on to the handshake. Regarding Python and PHP clients - can not we use some kind of timers to implement this feature? Best Regards, Igor On Mon, Feb 7, 2022 at 5:23 PM Pavel Tupitsyn <ptupit...@apache.org> wrote: > Maksim, agree. Let's not be too clever and only log a warning. > > On Mon, Feb 7, 2022 at 5:23 PM Pavel Tupitsyn <ptupit...@apache.org> > wrote: > > > Ivan, idleTimeout already exists, I don't think we should change the way > > it works (or did I misunderstand you?) > > > > Of course, enabling heartbeats means that otherwise idle clients will no > > longer be disconnected by the server. > > I think we should cross-link those properties in the documentation and > > explain this behavior. > > > > On Mon, Feb 7, 2022 at 4:39 PM Ivan Daschinsky <ivanda...@gmail.com> > > wrote: > > > >> >>3. Already implemented: when ClientConnectorConfiguration#idleTimeout > is > >> not zero, server disconnects idle clients > >> >> > >> But I suppose it would be great to have: > >> 1. If client supports keep alive, use idleTimeout > >> 2. If not, do not use it. > >> > >> But I am not sure if it is correct or not. > >> > >> пн, 7 февр. 2022 г. в 16:01, Maksim Timonin <timoninma...@apache.org>: > >> > >> > I believe explicit is better than implicit :) Also in case of dynamic > >> > calculation of timeout, it can change dynamically, for example > >> restarting a > >> > cluster with different configuration should reconfigure clients too. > >> Looks > >> > complicated. > >> > > >> > My vote for WARN + javadocs with mention of this issue. > >> > > >> > On Mon, Feb 7, 2022 at 3:51 PM Pavel Tupitsyn <ptupit...@apache.org> > >> > wrote: > >> > > >> > > > WDYT, should we add a WARN message for clients that configure > >> > > > keepAliveTimeout greater than idleTimeout on the server side? > >> > > > >> > > I think we should either log a WARN, or retrieve idleTimeout from > >> server > >> > > and configure heartbeatTimeout accordingly (e.g. divide by 2). > >> > > Thoughts? > >> > > > >> > > On Mon, Feb 7, 2022 at 3:14 PM Maksim Timonin < > >> timoninma...@apache.org> > >> > > wrote: > >> > > > >> > > > Hi Pavel, > >> > > > > >> > > > Thanks for the links. Yes, I forgot that the flag of changed > >> topology > >> > is > >> > > > lazy. Also I missed that the keepAlive setting is configured on > the > >> > > client > >> > > > side (alternatively to idleTimeout that is on the server side). > >> > > > > >> > > > Now I understand, this feature can be helpful then. Every client > can > >> > > > configure itself in case it's possible to be idle sometimes, and > >> choose > >> > > > an appropriate timeout by itself too. And by default the feature > >> should > >> > > be > >> > > > disabled. > >> > > > > >> > > > WDYT, should we add a WARN message for clients that configure > >> > > > keepAliveTimeout greater than idleTimeout on the server side? > >> > > > > >> > > > > >> > > > > >> > > > On Mon, Feb 7, 2022 at 1:05 PM Pavel Tupitsyn < > ptupit...@apache.org > >> > > >> > > > wrote: > >> > > > > >> > > > > Ivan, > >> > > > > > >> > > > > I suggest the following: > >> > > > > > >> > > > > 1. Server sends KEEP_ALIVE feature flag, which means it accepts > >> > > > > OP_KEEP_ALIVE empty message > >> > > > > 2. Client sends OP_KEEP_ALIVE when the connection is idle for a > >> > > > > certain period of time > >> > > > > 3. Already implemented: when > >> ClientConnectorConfiguration#idleTimeout > >> > > is > >> > > > > not zero, server disconnects idle clients > >> > > > > > >> > > > > This way we don't need server->client keepalives, as you > correctly > >> > > noted. > >> > > > > > >> > > > > On Mon, Feb 7, 2022 at 12:43 PM Ivan Daschinsky < > >> ivanda...@gmail.com > >> > > > >> > > > > wrote: > >> > > > > > >> > > > > > Pavel, I suppose that ideally: > >> > > > > > 1. Client send in handshake flag, that it supports KEEP_ALIVE > >> > feature > >> > > > and > >> > > > > > server takes it into account. > >> > > > > > 2. Each request of client can be considered as keep-alive > ping. > >> > > > > > 3. Client send failure should be processed using retry policy. > >> > > > > > 4. Server should not send keep-alive packets, it is redundant, > >> but > >> > > > server > >> > > > > > should track requests from client and if there is no requests > >> from > >> > > > client > >> > > > > > with KEEP_ALIVE feature, > >> > > > > > automatically close connection and free resources. > >> > > > > > > >> > > > > > Similar approach is used in zookeeper clients. > >> > > > > > > >> > > > > > пн, 7 февр. 2022 г. в 12:24, Pavel Tupitsyn < > >> ptupit...@apache.org > >> > >: > >> > > > > > > >> > > > > > > Ivan, > >> > > > > > > > >> > > > > > > Ideally, the check should come from both sides. > >> > > > > > > - Client periodically sends keepalive to server > >> > > > > > > - Server periodically sends keepalive to client > >> > > > > > > > >> > > > > > > Feature flags will be added accordingly, so it is not > >> necessary > >> > to > >> > > > > > > implement this in all thin clients. > >> > > > > > > > >> > > > > > > On Mon, Feb 7, 2022 at 11:43 AM Ivan Daschinsky < > >> > > ivanda...@gmail.com > >> > > > > > >> > > > > > > wrote: > >> > > > > > > > >> > > > > > > > I suppose it is great idea, but this functionality can be > >> hard > >> > to > >> > > > > > > implement > >> > > > > > > > for some platforms. I.e. sync python client or php (there > >> is no > >> > > > real > >> > > > > > > > multithreading for python (GIL) and php is single threaded > >> by > >> > > > > design). > >> > > > > > > But > >> > > > > > > > for async clients it is not very hard to implement. > >> > Nevertheless, > >> > > > > this > >> > > > > > > > feature should be optional, because of possible technical > >> > > > > limitations. > >> > > > > > > > > >> > > > > > > > Pavel, is this check mostly for client side? Or servers > can > >> do > >> > > some > >> > > > > > > actions > >> > > > > > > > if there is no activity from thin client (i.e. closing > >> context > >> > > and > >> > > > > free > >> > > > > > > > resources such as queries' handles and so on?) > >> > > > > > > > > >> > > > > > > > пн, 7 февр. 2022 г. в 11:09, Pavel Tupitsyn < > >> > > ptupit...@apache.org > >> > > > >: > >> > > > > > > > > >> > > > > > > > > Hi Maksim, > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > half-state is a possible situation when an Ignite node > >> goes > >> > > > down > >> > > > > or > >> > > > > > > > > somehow removes connection to a thin client > >> > > > > > > > > > >> > > > > > > > > Half-open state is also possible when, for example, an > >> > > > intermediate > >> > > > > > > > router > >> > > > > > > > > is rebooted [1]. > >> > > > > > > > > > >> > > > > > > > > This is what we seem to have encountered with one of our > >> > > > customers > >> > > > > - > >> > > > > > > they > >> > > > > > > > > have a stable cluster, and long-living (multiple days) > >> thin > >> > > > client > >> > > > > > > > > connections which can be idle for some time. > >> > > > > > > > > And only when we send some data on such an idle > >> connection do > >> > > we > >> > > > > > > discover > >> > > > > > > > > that it is broken. > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > But with enabled (true by default) partitionAwareness > >> > feature > >> > > > > > clients > >> > > > > > > > can > >> > > > > > > > > be notified about topology changes > >> > > > > > > > > > >> > > > > > > > > Partition awareness is a "lazy" notification in a form > of > >> a > >> > > > > response > >> > > > > > > > > message flag [2]. > >> > > > > > > > > You won't get one on an idle connection. > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > the connections are removed on the server side by > client > >> > idle > >> > > > > > timeout > >> > > > > > > > > > >> > > > > > > > > Idle timeout is disabled by default. > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > is it OK to keep such connections alive for a long > time > >> > > > > > > > > > >> > > > > > > > > I think it is up to the user. > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > in the case of partition awareness features it can > lead > >> to > >> > > > > wasting > >> > > > > > > TCP > >> > > > > > > > > sockets on Ignite nodes, can't it > >> > > > > > > > > > >> > > > > > > > > Can you please elaborate? > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > [1] > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > https://blog.stephencleary.com/2009/05/detection-of-half-open-dropped.html > >> > > > > > > > > [2] > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > https://cwiki.apache.org/confluence/display/IGNITE/IEP-23%3A+Best+Effort+Affinity+for+Thin+Clients > >> > > > > > > > > > >> > > > > > > > > On Fri, Feb 4, 2022 at 4:01 PM Maksim Timonin < > >> > > > > > timoninma...@apache.org > >> > > > > > > > > >> > > > > > > > > wrote: > >> > > > > > > > > > >> > > > > > > > > > Hi Pavel, > >> > > > > > > > > > > >> > > > > > > > > > Thanks for starting this thread! Can I ask some > >> questions > >> > > here > >> > > > to > >> > > > > > get > >> > > > > > > > the > >> > > > > > > > > > feature more clearly? > >> > > > > > > > > > > >> > > > > > > > > > As I understand it correctly, half-state is a possible > >> > > > situation > >> > > > > > when > >> > > > > > > > an > >> > > > > > > > > > Ignite node goes down or somehow removes connection > to a > >> > thin > >> > > > > > client. > >> > > > > > > > But > >> > > > > > > > > > with enabled (true by default) partitionAwareness > >> feature > >> > > > clients > >> > > > > > can > >> > > > > > > > be > >> > > > > > > > > > notified about topology changes. So, there are > possible > >> > > cases: > >> > > > > > > > > > 1. ThinClient connects to a single node. > >> > > > > > > > > > 2. Ignite node removes connection from itself. > >> > > > > > > > > > > >> > > > > > > > > > I like the idea for the case with a single node, as it > >> > helps > >> > > > fail > >> > > > > > > fast. > >> > > > > > > > > > But is it OK to connect a client to a single node > only? > >> > > > > > > > > > > >> > > > > > > > > > For the second one: you mention that a case for the > >> second > >> > > > option > >> > > > > > is > >> > > > > > > > > > "Long-living and mostly idle connections are > especially > >> > > > > susceptible > >> > > > > > > to > >> > > > > > > > > this > >> > > > > > > > > > behavior". If I understand correctly the connections > are > >> > > > removed > >> > > > > on > >> > > > > > > the > >> > > > > > > > > > server side by client idle timeout. Can we just > >> configure > >> > the > >> > > > > idle > >> > > > > > > > > timeout > >> > > > > > > > > > for cases where we really need keeping alive idle > >> > > connections? > >> > > > > Are > >> > > > > > > > there > >> > > > > > > > > > any other cases with unexpectedly dropped connections? > >> > > > > > > > > > > >> > > > > > > > > > I'm wondering is it OK to keep such connections alive > >> for a > >> > > > long > >> > > > > > > time? > >> > > > > > > > > > Also in the case of partition awareness features it > can > >> > lead > >> > > to > >> > > > > > > wasting > >> > > > > > > > > TCP > >> > > > > > > > > > sockets on Ignite nodes, can't it? > >> > > > > > > > > > > >> > > > > > > > > > Thanks! > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > On Thu, Feb 3, 2022 at 2:24 PM Pavel Tupitsyn < > >> > > > > > ptupit...@apache.org> > >> > > > > > > > > > wrote: > >> > > > > > > > > > > >> > > > > > > > > >> Igniters, > >> > > > > > > > > >> > >> > > > > > > > > >> Please review the proposal to add heartbeat messages > to > >> > the > >> > > > thin > >> > > > > > > > client > >> > > > > > > > > >> protocol (both 2.x and 3.x) and let me know your > >> thoughts: > >> > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > > >> > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > https://cwiki.apache.org/confluence/display/IGNITE/IEP-83+Thin+Client+Keepalive > >> > > > > > > > > >> > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > -- > >> > > > > > > > Sincerely yours, Ivan Daschinskiy > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> > > > > > -- > >> > > > > > Sincerely yours, Ivan Daschinskiy > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> > >> -- > >> Sincerely yours, Ivan Daschinskiy > >> > > >
