>> idleTimeout already exists, I don't think we should change the way it works (or did I misunderstand you?) If we use new approach, we can reduce this timeout. But this can affect old clients.
Also, let's think about that sending heartbeats and interval of sending heartbeats could be calculated on the server side (i.e. one third of idle timeout) and sent to the client during handshake. Also we can introduce something like a negotiation mechanism as in zookeeper. пн, 7 февр. 2022 г. в 18:05, Pavel Tupitsyn <ptupit...@apache.org>: > Igor, > > > Maybe clients should pass this information on to the handshake. > > Do you think we should log a mismatched timeout warning on the server, not > on the client? > Or should we do both? > > > I've updated the proposal with OP_GET_IDLE_TIMEOUT and some other details > discussed above. > > On Mon, Feb 7, 2022 at 5:42 PM Igor Sapego <isap...@apache.org> wrote: > > > 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 > > > >> > > > > > > > > > > -- Sincerely yours, Ivan Daschinskiy
