Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session & connection timeout properties; Adding CuratorFrameworkCusto… #244

Closed
wants to merge 1 commit into from

Conversation

berngp
Copy link
Contributor

@berngp berngp commented Mar 19, 2020

We currently don't have a simple mechanism to configure CuratorFramework's session and connection timeout. Since Curator 3.x users can affect how Curator handles re-connection, since the meaning of LOST was changed. In 3.x when the "Disconnected" is received Curator starts an internal timer. When the timer passes the negotiated session timeout Curator calls getTestable().injectSessionExpiration()
and posts a LOST state change.

In Curator 4.x the session timeout defaults to 60 seconds and the connection timeout to 15 seconds.
Ref:

private static final int DEFAULT_SESSION_TIMEOUT_MS = Integer.getInteger("curator-default-session-timeout", 60 * 1000);
private static final int DEFAULT_CONNECTION_TIMEOUT_MS = Integer.getInteger("curator-default-connection-timeout", 15 * 1000);

Proposed Solution

Offer a mechanism to configure the session and connection timeout and establish a mechanism to be able to customize the CuratorFramework for future-proofing.

We could avoid having two more properties, the session and connection timeouts, as part of ZookeeperProperties and let users define them on their CuratorFrameworkCustomizer. The current rational is that consolidating this two configuration properties as part of the spring.cloud.zookeeper configuration namespace is valuable and simplifies how they are used.

Additional Context

Curator will set the LOST state when it believes that the ZooKeeper session has expired.
ZooKeeper connections have a session. When the session expires, clients must take appropriate action.
In Curator, this is complicated by the fact that Curator internally manages the ZooKeeper connection.
Curator will set the LOST state when any of the following occurs:
a) ZooKeeper returns a Watcher.Event.KeeperState.Expired or KeeperException.Code.SESSIONEXPIRED;
b) Curator closes the internally managed ZooKeeper instance;
c) The session timeout elapses during a network partition. It is possible to get a RECONNECTED state after
this but you should still consider any locks, etc. as dirty/unstable.
Ref. Curator Errors

Warning: Curator 4.0.1 is affected by CURATOR-460: Timed tolerance for connection suspended leads to simultaneous leaders. This was addressed in 4.1.0.

Additional references:

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a few small changes.

…mizer

We currently don't have a simple mechanism to configure CuratorFramework's session and connection timeout.
Since Curator 3.x users can affect how Curator handles re-connection, since the meaning of LOST was changed.
In 3.x when the "Disconnected" is received Curator starts an internal timer.
When the timer passes the negotiated session timeout Curator calls `getTestable().injectSessionExpiration()`
and posts a LOST state change.

In Curator 4.x the session timeout defaults to 60 seconds and the connection timeout to 15 seconds.
Ref:
```
private static final int DEFAULT_SESSION_TIMEOUT_MS = Integer.getInteger("curator-default-session-timeout", 60 * 1000);
private static final int DEFAULT_CONNECTION_TIMEOUT_MS = Integer.getInteger("curator-default-connection-timeout", 15 * 1000);
```

Proposed Solution
=================

Offer a mechanism to configure the session and connection timeout and
establish a mechanism to be able to customize the `CuratorFramework` for
future-proving.

We could avoid having two more properties, the session and connection timeouts, as part of `ZookeeperProperties` and
let users define them on their `CuratorFrameworkCustomizer`. The current rational is that consolidating this two
configuration properties as part of the `spring.cloud.zookeeper` configuration namespace is valuable and simplifies
their usage.

Additional Context
==================

> Curator will set the LOST state when it believes that the ZooKeeper session has expired.
> ZooKeeper connections have a session. When the session expires, clients must take appropriate action.
> In Curator, this is complicated by the fact that Curator internally manages the ZooKeeper connection.
> Curator will set the LOST state when any of the following occurs:
> a) ZooKeeper returns a Watcher.Event.KeeperState.Expired or KeeperException.Code.SESSIONEXPIRED;
> b) Curator closes the internally managed ZooKeeper instance;
> c) The session timeout elapses during a network partition. It is possible to get a RECONNECTED state after
> this but you should still consider any locks, etc. as dirty/unstable.
Ref. [Curator Errors](https://curator.apache.org/errors.html)

Warning: Curator 4.0.1 is affected by [CURATOR-460: Timed tolerance for connection suspended leads to simultaneous leaders](https://issues.apache.org/jira/browse/CURATOR-460). This was addressed in `4.1.0`.

Additional references:
=====================
* [TN14: ZooKeeper's Session Handling](https://cwiki.apache.org/confluence/display/CURATOR/TN14)
* [CURATOR-460](https://issues.apache.org/jira/browse/CURATOR-460)
* [Curator Errors](https://curator.apache.org/errors.html)
@spencergibb spencergibb added this to the 2.2.2.RELEASE milestone Apr 20, 2020
@berngp berngp deleted the feature/zk-properties branch April 20, 2020 20:29
@TJYS
Copy link

TJYS commented Jun 16, 2020

CuratorFrameworkCustomizer can't be injected when I use "@bean" ,It's sound like a problem about Loading order

@spencergibb
Copy link
Member

@TJYS we have tests that create beans that prove it works.

If you can provide a complete, minimal, verifiable sample that reproduces the problem, please open a new issue. It should be available as a GitHub (or similar) project or attached to the new issue as a zip file.

@TJYS
Copy link

TJYS commented Jun 30, 2020

@spencergibb if only "spring-cloud-starter-zookeeper-discovery" on the classpath, it works. BUT if I use "spring-cloud-starter-zookeeper-config" at the same time OR only use "spring-cloud-starter-zookeeper-config", it doesn't works. CuratorFrameworkCustomizer can't be injected.
I watched that "ZooKeeperAutoConfiguration" was loaded in different order.

@spencergibb
Copy link
Member

Please open a new issue

@TJYS
Copy link

TJYS commented Jun 30, 2020

OK! I had writen it. #249

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants