You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by Randgalt <gi...@git.apache.org> on 2015/08/23 18:06:43 UTC
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
GitHub user Randgalt opened a pull request:
https://github.com/apache/curator/pull/97
[CURATOR-247] Extend Curator's connection state to support SESSION_LOST
This is a significant change. Please review carefully and let's have a lot of discussion. Because the new behavior is so much better (and more consistent with expectations) <strong>I've enabled it by default</strong>.
Major differences from the older behavior are:
* Session/connection timeouts are no longer managed by the low-level client. They are managed by the CuratorFramework instance. There should be no noticeable differences.
* Prior to 3.0.0, each iteration of the retry policy would allow the connection timeout to elapse if the connection hadn't yet succeeded. This meant that the true connection timeout was the configured value times the maximum retries in the retry policy. This longstanding issue has been address. Now, the connection timeout can elapse only once for a single API call.
* MOST IMPORTANTLY! Prior to 3.0.0, ConnectionState.LOST did not imply a lost session (much to the confusion of users). Now, Curator will set the LOST state only 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. Now, 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 configured session timeout elapses during a network partition.
Something important to consider. Given the significance of this change it makes to have it be part of 3.0.0 but if we merge it into 3.0.0 now it will be harder to maintain master and 3.0.0 as separate branches. Some git expertise is needed here on how to manage this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/curator CURATOR-247
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/97.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #97
----
commit 344634ac6e34e61bc0cc7b41923a1df4089c7948
Author: randgalt <ra...@apache.org>
Date: 2015-08-21T17:10:24Z
First pass at new (optional) definition of state LOST
commit 2343daf29388566b0efa0b0a2ad21574fb534a27
Author: randgalt <ra...@apache.org>
Date: 2015-08-21T20:11:59Z
Merge branch 'CURATOR-3.0' into CURATOR-247
commit 62f3c33cdb556eccf6fe1cc87ee74b3458431777
Author: randgalt <ra...@apache.org>
Date: 2015-08-21T22:35:44Z
Continued work on new LOST behavior. Added some tests. To get correct behavior it's necessary to not retry connection failures. Retrying connection failures was never a good idea and here's a good opportunity to fix it as this requires client action to enable
commit c5a49216cc78b052066661a8ded357e50e0b6313
Author: randgalt <ra...@apache.org>
Date: 2015-08-21T22:37:15Z
license
commit d3170099757c7e17ff8fbee0c37d620aacb60d65
Author: randgalt <ra...@apache.org>
Date: 2015-08-21T22:49:55Z
more tests
commit b8d4c3d77de029917820634fa4ed21be19bbcf2c
Author: randgalt <ra...@apache.org>
Date: 2015-08-21T22:59:07Z
minor reformat
commit 847cc0d2415f59c2943d4a2734564119ffb38bb1
Author: randgalt <ra...@apache.org>
Date: 2015-08-22T15:47:01Z
wip
commit ec2f9bd555d01b324bd5ef690b1036d98e1f3702
Author: randgalt <ra...@apache.org>
Date: 2015-08-22T16:06:33Z
Fixed testRetry() for new LOST behavior
commit 6381ccb6536f4710248a50ae5d0313399bbfe858
Author: randgalt <ra...@apache.org>
Date: 2015-08-22T22:50:09Z
removed some test code
commit e239137019608f02cabb23c27ab13adcef88c027
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T00:06:55Z
major refactoring. Abstracting old/new behavior into a pluggable ConnectionHandlingPolicy. Also, IMPORTANT, made the new behavior the default. This needs to be discussed but it's a major improvement and we should default to it.
commit 30bd7b655d201762d8ff74062964621879ac7134
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T00:29:36Z
further refactoring. Abstracted old framework-level connection handling into ClassicInternalConnectionHandler. Probably more to do here
commit 23554479597d654fa8318cdc579fc3cc29bc2c54
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T01:10:34Z
Curator has a big problem with thread interrupted states getting cleared. There are several issues on this (CURATOR-208, CURATOR-205, CURATOR-228, CURATOR-109
commit 05d241da642c6ba0d16b3ce97557128fad4dfe41
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T01:32:41Z
When the connection timeout elapses and there is more than one server in the connection string, reset the connection and try again
commit face4034e9fdcc9ffdb394c7c1682add834a1e10
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T02:54:24Z
Longer connection timeout needed
commit 5f094f8bb6dca3c056051cb8800b418839cca0e1
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T12:49:17Z
More refinement of classic/new connection handling. Reworked how the retry policy is invoked for each. New behavior is now confirmed to be: wait for connection timeout only once. Some tests will need work due to this
commit e001e0098f64baa8e0b3b887507bc98972c775dc
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T14:33:46Z
more work on repairing tests for new connection handling
commit 1a2a94b625e7e1b5e535414e397e9b3a4173ca1b
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T15:54:29Z
more work on repairing tests for new connection handling
commit 64d966c18b9d18c40e13fda98e52d9253b281086
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T15:57:48Z
doc
commit 9c7cf5d8ba495bccdea2bcb6b377e95f5f99d521
Author: randgalt <ra...@apache.org>
Date: 2015-08-23T16:02:19Z
doc
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133976579
> Looks like you can call getSessionTimeout() on the ZooKeeper object and it returns the negotiated session timeout. You would need to call it every time a connection / reconnection occurred.
Excellent. I'll do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133976670
> Assume a timing discrepancy between the curator client and ZK server, such that the client assumes a session is lost by timeout, but in a (racy) way, the connection is successfully re-established to the server and the server does not drop the session. Can we be sure to resolve this race, such that if session lost events have already fired or started firing, the client forcibly creates a new session instead of using the old one?
This isn't an issue because when the LOST state is generated the current client is also closed so the session will expire in any event.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133976279
> Isn't there some sort of negotiation when connecting to Zookeeper in regards to session timeout?
Oh right. Good point. I don't think that value is accessible on the client side. But, it could be calculated if we had the server configuration values. I think we can get those through the new config api right? I'll look into this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133990812
This is hilarious. In July 2013, I submitted a patch to add some testable hooks to ZooKeeper. I forgot all about it was never touched (https://issues.apache.org/jira/browse/ZOOKEEPER-1730). Well, it's now merged in to ZK 3.5 and, guess what, it has a SESSION expiration injection method! This is Twilight Zone level stuff. I was on my computer writing a hack to inject the session event and digging into ZooKeeper source when I saw what looked like my code. Imagine my surprise...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133976436
Looks like you can call getSessionTimeout() on the ZooKeeper object and it returns the negotiated session timeout. You would need to call it every time a connection / reconnection occurred.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-135190182
Let's wait until you and Mike have a look if you both want.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133959712
Nice work Jordan,
I just had a quick preliminary look through stuff and I had one observation. For the timing stuff that checks if we've been in a SUSPENDED state for long enough for the server to have timed out the connection, it's using the session timeout specified in the builder.
Isn't there some sort of negotiation when connecting to Zookeeper in regards to session timeout? I.e. It's possible for Zookeeper to specify that a different session timeout will be used rather than the one that the client wants?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt closed the pull request at:
https://github.com/apache/curator/pull/97
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/97#discussion_r38378657
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java ---
@@ -103,23 +106,34 @@
public CuratorFrameworkImpl(CuratorFrameworkFactory.Builder builder)
{
ZookeeperFactory localZookeeperFactory = makeZookeeperFactory(builder.getZookeeperFactory());
- this.client = new CuratorZookeeperClient(localZookeeperFactory, builder.getEnsembleProvider(), builder.getSessionTimeoutMs(), builder.getConnectionTimeoutMs(), new Watcher()
- {
- @Override
- public void process(WatchedEvent watchedEvent)
- {
- CuratorEvent event = new CuratorEventImpl(CuratorFrameworkImpl.this, CuratorEventType.WATCHED, watchedEvent.getState().getIntValue(), unfixForNamespace(watchedEvent.getPath()), null, null, null, null, null, watchedEvent, null, null);
- processEvent(event);
- }
- }, builder.getRetryPolicy(), builder.canBeReadOnly());
+ this.client = new CuratorZookeeperClient
+ (
+ localZookeeperFactory,
+ builder.getEnsembleProvider(),
+ builder.getSessionTimeoutMs(),
+ builder.getConnectionTimeoutMs(),
+ new Watcher()
+ {
+ @Override
+ public void process(WatchedEvent watchedEvent)
+ {
+ CuratorEvent event = new CuratorEventImpl(CuratorFrameworkImpl.this, CuratorEventType.WATCHED, watchedEvent.getState().getIntValue(), unfixForNamespace(watchedEvent.getPath()), null, null, null, null, null, watchedEvent, null, null);
+ processEvent(event);
+ }
+ },
+ builder.getRetryPolicy(),
+ builder.canBeReadOnly(),
+ builder.getConnectionHandlingPolicy()
+ );
+ internalConnectionHandler = builder.getConnectionHandlingPolicy().isEmulatingClassicHandling() ? new ClassicInternalConnectionHandler() : new StandardInternalConnectionHandler();
--- End diff --
Is there a reason that this needs to be a separate instance rather than just a reference to the connection handler from the builder?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/97#discussion_r38379208
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -240,27 +248,37 @@ private void processEvents()
{
try
{
- while ( !Thread.currentThread().isInterrupted() )
+ while ( !Thread.currentThread().isInterrupted() && (state.get() == State.STARTED) )
{
- final ConnectionState newState = eventQueue.take();
-
- if ( listeners.size() == 0 )
+ int pollMaxMs = (sessionTimeoutMs * 2) / 3; // 2/3 of session timeout
--- End diff --
This is the configured session timeout MS. Will this cause issues if the negotiated timeout is different?
I guess it just means that it will potentially take 4/3 * configured session timeout to determine if the session has timed out during network partition?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133976636
> Are we able to precisely articulate the relationship between session lost and existing watchers
Excellent point. I missed that. I just did a test and, unfortunately, when the client is closed the watchers are not notified. I'll need to emulate a session expiration event of some kind. I'll look into this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133910125
Also, FYI, each test is now run twice. Once with the old connection handling and once with the new handling.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-136549886
Other than those minor comments, I think this looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-135181795
I haven't given it more than a cursory look. If Scott's had a proper look and is OK with it then I'm happy, otherwise I can have a look, but it's probably not going to be for a few days.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/97#discussion_r38379346
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -240,27 +248,37 @@ private void processEvents()
{
try
{
- while ( !Thread.currentThread().isInterrupted() )
+ while ( !Thread.currentThread().isInterrupted() && (state.get() == State.STARTED) )
{
- final ConnectionState newState = eventQueue.take();
-
- if ( listeners.size() == 0 )
+ int pollMaxMs = (sessionTimeoutMs * 2) / 3; // 2/3 of session timeout
--- End diff --
Good point. I should use the negotiated timeout.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-135157256
Any more thoughts on this? Can it be merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/97#discussion_r38377860
--- Diff: curator-client/src/main/java/org/apache/curator/connection/ConnectionHandlingPolicy.java ---
@@ -0,0 +1,72 @@
+package org.apache.curator.connection;
+
+import org.apache.curator.CuratorZookeeperClient;
+import org.apache.curator.RetryLoop;
+import java.util.concurrent.Callable;
+
+/**
+ * Abstracts connection handling so that Curator can emulate it's old, pre 3.0.0
+ * handling and update to newer handling.
+ */
+public interface ConnectionHandlingPolicy
+{
+ /**
+ * Return true if this policy should behave like the pre-3.0.0 version of Curator
+ *
+ * @return true/false
+ */
+ boolean isEmulatingClassicHandling();
+
+ /**
+ * Called by {@link RetryLoop#callWithRetry(CuratorZookeeperClient, Callable)} to do the work
+ * of retrying
+ *
+ * @param client client
+ * @param proc the procedure to retry
+ * @return result
+ * @throws Exception errors
+ */
+ <T> T callWithRetry(CuratorZookeeperClient client, Callable<T> proc) throws Exception;
+
+ enum CheckTimeoutsResult
+ {
+ /**
+ * Do nothing
+ */
+ NOP,
+
+ /**
+ * handle a new connection string
+ */
+ NEW_CONNECTION_STRING,
+
+ /**
+ * reset/recreate the internal ZooKeeper connection
+ */
+ RESET_CONNECTION,
+
+ /**
+ * handle a connection timeout
+ */
+ CONNECTION_TIMEOUT,
+
+ /**
+ * handle a session timeout
+ */
+ SESSION_TIMEOUT
+ }
+
+ /**
+ * Check timeouts. NOTE: this method is only called when an attempt to access to the ZooKeeper instances
+ * is made and the connection has not completed.
+ *
+ * @param hasNewConnectionString proc to call to check if there is a new connection string. Important: the internal state is cleared after
+ * this is called so you MUST handle the new connection string if <tt>true</tt> is returned
+ * @param connectionStartMs the epoch/ms time that the connection was first initiated
+ * @param sessionTimeoutMs the configured session timeout in milliseconds
--- End diff --
Should this be the negotiated session timeout rather than the configured session timeout? Looking at the code that calls into this interface, it would seem that the configured timeout from the build is being used. Is that correct though? I would have thought that the negotiated timeout was what was important as it's the source of truth.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request: [CURATOR-247] Extend Curator's connection st...
Posted by dragonsinth <gi...@git.apache.org>.
Github user dragonsinth commented on the pull request:
https://github.com/apache/curator/pull/97#issuecomment-133928010
Very cool. Before I dive in, couple of high level questions.
1) Are we able to precisely articulate the relationship between session lost and existing watchers? For example, an acceptable answer might be "When a session is lost, all outstanding watchers will fire a lost event. Afterwards, all watchers will be removed. Client code should create new watchers if the session is re-established. If a connection can be re-established before the session is lost, all watchers will remain active."
2) Assume a timing discrepancy between the curator client and ZK server, such that the client assumes a session is lost by timeout, but in a (racy) way, the connection is successfully re-established to the server and the server does not drop the session. Can we be sure to resolve this race, such that if session lost events have already fired or started firing, the client forcibly creates a new session instead of using the old one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---