You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by arrodrigues <gi...@git.apache.org> on 2018/04/03 02:46:22 UTC
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
GitHub user arrodrigues opened a pull request:
https://github.com/apache/curator/pull/262
[CURATOR-460] Timed tolerance for connection suspended leads to simultaneous leaders
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arrodrigues/curator CURATOR-460
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/curator/pull/262.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 #262
----
commit 72b7b8709c822b9170d8d7beb2814c4c36b89e58
Author: javando <an...@...>
Date: 2018-04-03T02:27:13Z
CURATOR-460 Adjust to obey sessionExpirationPercent value
----
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179319128
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -253,6 +253,7 @@ private void processEvents()
{
int lastNegotiatedSessionTimeoutMs = client.getZookeeperClient().getLastNegotiatedSessionTimeoutMs();
int useSessionTimeoutMs = (lastNegotiatedSessionTimeoutMs > 0) ? lastNegotiatedSessionTimeoutMs : sessionTimeoutMs;
+ useSessionTimeoutMs = sessionExpirationPercent > 0 && startOfSuspendedEpoch != 0 ? (useSessionTimeoutMs * sessionExpirationPercent) / 100 : useSessionTimeoutMs;
--- End diff --
Therefore, I'm going to synchronize the access at that point.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
Thanks for the updates, I think this looks good.
I still think we need to look at the default behaviour of the ConnectionStateListener though. Currently, a LOST event will only be injected after 4/3 of the session timeout (+ some small error). I think that this default behaviour should be changed so that the LOST event is injected at session timeout (+ some small error). This could be achieved by either changing the StandardConnectionHandlingPolicy default constructor to use 66 instead of 100 for the %, or by changing the ConnectionStateListener logic. I think it's probably simpler to change via the StandardConnectionHandlingPolicy, because the ConnectionStateListener logic interacts with the % defined in the StandardConnectionHandlingPolicy and has more potential to break existing use cases.
What are your thoughts @Randgalt ?
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
Ok, will merge this now.
Thanks @arrodrigues !
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179001451
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java ---
@@ -222,6 +223,35 @@ public void notLeader()
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
+
+ latch.close();
+ client.close();
+
+ timing.sleepABit();
+ states.clear();
+ server = new TestingServer();
+ client = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .connectionTimeoutMs(1000)
+ .sessionTimeoutMs(timing.session())
+ .retryPolicy(new RetryOneTime(1))
+ .connectionStateErrorPolicy(new SessionConnectionStateErrorPolicy())
+ .connectionHandlingPolicy(new StandardConnectionHandlingPolicy(30))
+ .build();
+ client.getConnectionStateListenable().addListener(stateListener);
+ client.start();
+ latch = new LeaderLatch(client, "/test");
+ latch.addListener(listener);
+ latch.start();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.CONNECTED.name());
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), "true");
+ server.close();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.SUSPENDED.name());
+ next = Lists.newArrayList();
+
+ next.add(states.poll(timing.session() / 3, TimeUnit.MILLISECONDS));
+ next.add(states.poll(timing.forSleepingABit().milliseconds(), TimeUnit.MILLISECONDS));
+ Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
--- End diff --
The issue isn't really the LeaderLatch specifically is it? I think it's probably cleaner to create a test case under curator-framework for the ConnectionStateManager and then have a test explicitly for injected LOST events, rather than have something in the LeaderLatch tests.
Because, this test doesn't really cover the use case that was discussed originally (2 clients being leader at the same time).
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/262
I'd be -1 on more hacking into ZooKeeper. Trying to get all these timings to be accurate is a lost cause anyway - we can never simulate in the client the exact timing of the server. I suggest adding a configurable (via CLI) "slosh" value or something like that.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179022253
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java ---
@@ -222,6 +223,35 @@ public void notLeader()
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
+
+ latch.close();
+ client.close();
+
+ timing.sleepABit();
+ states.clear();
+ server = new TestingServer();
+ client = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .connectionTimeoutMs(1000)
+ .sessionTimeoutMs(timing.session())
+ .retryPolicy(new RetryOneTime(1))
+ .connectionStateErrorPolicy(new SessionConnectionStateErrorPolicy())
+ .connectionHandlingPolicy(new StandardConnectionHandlingPolicy(30))
+ .build();
+ client.getConnectionStateListenable().addListener(stateListener);
+ client.start();
+ latch = new LeaderLatch(client, "/test");
+ latch.addListener(listener);
+ latch.start();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.CONNECTED.name());
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), "true");
+ server.close();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.SUSPENDED.name());
+ next = Lists.newArrayList();
+
+ next.add(states.poll(timing.session() / 3, TimeUnit.MILLISECONDS));
+ next.add(states.poll(timing.forSleepingABit().milliseconds(), TimeUnit.MILLISECONDS));
+ Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
--- End diff --
I think that the existing test case is trying to test connection / session loss cases in the context of the LeaderLatch recipe (thus the checks for "true" and "false"), whereas for this new test case, we don't really care about the fact that a LeaderLatch is running, rather that the ConnectionStateManager is doing the right thing. So, I'd prefer to see a standalone test in a TestConnectionStateManager class.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179396466
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -251,9 +251,15 @@ private void processEvents()
{
try
{
+
+ long localStartOfSuspendedEpoch;
+ synchronized (this) {
+ localStartOfSuspendedEpoch = this.startOfSuspendedEpoch;
+ }
int lastNegotiatedSessionTimeoutMs = client.getZookeeperClient().getLastNegotiatedSessionTimeoutMs();
int useSessionTimeoutMs = (lastNegotiatedSessionTimeoutMs > 0) ? lastNegotiatedSessionTimeoutMs : sessionTimeoutMs;
- long elapsedMs = startOfSuspendedEpoch == 0 ? useSessionTimeoutMs / 2 : System.currentTimeMillis() - startOfSuspendedEpoch;
+ useSessionTimeoutMs = sessionExpirationPercent > 0 && localStartOfSuspendedEpoch != 0 ? (useSessionTimeoutMs * sessionExpirationPercent) / 100 : useSessionTimeoutMs;
--- End diff --
The `sessionExpirationPercent` calc is the same as in the other spot in the code right? We can abstract a method that does this.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179319231
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java ---
@@ -222,6 +223,35 @@ public void notLeader()
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
+
+ latch.close();
+ client.close();
+
+ timing.sleepABit();
+ states.clear();
+ server = new TestingServer();
+ client = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .connectionTimeoutMs(1000)
+ .sessionTimeoutMs(timing.session())
+ .retryPolicy(new RetryOneTime(1))
+ .connectionStateErrorPolicy(new SessionConnectionStateErrorPolicy())
+ .connectionHandlingPolicy(new StandardConnectionHandlingPolicy(30))
+ .build();
+ client.getConnectionStateListenable().addListener(stateListener);
+ client.start();
+ latch = new LeaderLatch(client, "/test");
+ latch.addListener(listener);
+ latch.start();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.CONNECTED.name());
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), "true");
+ server.close();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.SUSPENDED.name());
+ next = Lists.newArrayList();
+
+ next.add(states.poll(timing.session() / 3, TimeUnit.MILLISECONDS));
+ next.add(states.poll(timing.forSleepingABit().milliseconds(), TimeUnit.MILLISECONDS));
+ Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
--- End diff --
OK. I'm going to adjust that.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179652355
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -340,4 +338,12 @@ private void setCurrentConnectionState(ConnectionState newConnectionState)
currentConnectionState = newConnectionState;
startOfSuspendedEpoch = (currentConnectionState == ConnectionState.SUSPENDED) ? System.currentTimeMillis() : 0;
}
+
+ private synchronized int getUseSessionTimeoutMs() {
--- End diff --
Yes. I had this discussion with @cammckenzie some comments ago. Every access to startOfSuspendedEpoch is synchronized on "this" except this point, we could make it volatile or sync this point more, so we choose to sync it. In the first version I synchronized just the line accessing startOfSuspendedEpoch, but after adding getUseSessionTimeoutMs I tought it'd be more readable sync the whole method.
Do you prefer to make it volatile?
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
My issue is the difference in timing in these events:
-If the connection between client and server is lost a disconnected event is received essentially immediately.
-If a heart beat is missed it takes 2/3 of the session timeout.
So, we can't reliably do anything with timers because we can't tell the difference between these two events. In the first case, ideally, we would inject a LOST event after the negotiated session timeout MS. In the second case, ideally, we would inject a LOST event after 1/3 of the negotiated session timeout MS.
For a short session timeout the difference is minimal, but for a longer session timeout, it will be significant. I guess all we can do is the approach that we already have where the client of the Curator library can decide what % of the session timeout they want to use before a session timeout is injected. I would think that in general, connection loss is more likely than missed heart beats, so maybe we should just leave the default % of session timeout as it is at 100% as this will be correct for the most likely case.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179650717
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -251,9 +251,15 @@ private void processEvents()
{
try
{
+
+ long localStartOfSuspendedEpoch;
+ synchronized (this) {
+ localStartOfSuspendedEpoch = this.startOfSuspendedEpoch;
+ }
int lastNegotiatedSessionTimeoutMs = client.getZookeeperClient().getLastNegotiatedSessionTimeoutMs();
int useSessionTimeoutMs = (lastNegotiatedSessionTimeoutMs > 0) ? lastNegotiatedSessionTimeoutMs : sessionTimeoutMs;
- long elapsedMs = startOfSuspendedEpoch == 0 ? useSessionTimeoutMs / 2 : System.currentTimeMillis() - startOfSuspendedEpoch;
+ useSessionTimeoutMs = sessionExpirationPercent > 0 && localStartOfSuspendedEpoch != 0 ? (useSessionTimeoutMs * sessionExpirationPercent) / 100 : useSessionTimeoutMs;
--- End diff --
I think you mean useSessionTimeoutMs calc. I created a method for that. I'm not very happy with the name I gave to it "getUseSessionTimeoutMs", suggestions are certainly welcome.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/262
The ZK client code sends Disconnected when the Server disconnects or the server misses a heartbeat. So, that should be sufficient right?
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179001383
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -253,6 +253,7 @@ private void processEvents()
{
int lastNegotiatedSessionTimeoutMs = client.getZookeeperClient().getLastNegotiatedSessionTimeoutMs();
int useSessionTimeoutMs = (lastNegotiatedSessionTimeoutMs > 0) ? lastNegotiatedSessionTimeoutMs : sessionTimeoutMs;
+ useSessionTimeoutMs = sessionExpirationPercent > 0 && startOfSuspendedEpoch != 0 ? (useSessionTimeoutMs * sessionExpirationPercent) / 100 : useSessionTimeoutMs;
--- End diff --
I think you're right about synchronization of access to the startOfSuspendedEpoch here. It can be modified via the addStateChange and setToSuspended methods (which already synchronize access to it). I wonder if it's cleaner just to make startOfSuspendedEpoch volatile?
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179016314
--- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatch.java ---
@@ -222,6 +223,35 @@ public void notLeader()
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
next.add(states.poll(timing.forSessionSleep().milliseconds(), TimeUnit.MILLISECONDS));
Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
+
+ latch.close();
+ client.close();
+
+ timing.sleepABit();
+ states.clear();
+ server = new TestingServer();
+ client = CuratorFrameworkFactory.builder()
+ .connectString(server.getConnectString())
+ .connectionTimeoutMs(1000)
+ .sessionTimeoutMs(timing.session())
+ .retryPolicy(new RetryOneTime(1))
+ .connectionStateErrorPolicy(new SessionConnectionStateErrorPolicy())
+ .connectionHandlingPolicy(new StandardConnectionHandlingPolicy(30))
+ .build();
+ client.getConnectionStateListenable().addListener(stateListener);
+ client.start();
+ latch = new LeaderLatch(client, "/test");
+ latch.addListener(listener);
+ latch.start();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.CONNECTED.name());
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), "true");
+ server.close();
+ Assert.assertEquals(states.poll(timing.forWaiting().milliseconds(), TimeUnit.MILLISECONDS), ConnectionState.SUSPENDED.name());
+ next = Lists.newArrayList();
+
+ next.add(states.poll(timing.session() / 3, TimeUnit.MILLISECONDS));
+ next.add(states.poll(timing.forSleepingABit().milliseconds(), TimeUnit.MILLISECONDS));
+ Assert.assertTrue(next.equals(Arrays.asList(ConnectionState.LOST.name(), "false")) || next.equals(Arrays.asList("false", ConnectionState.LOST.name())), next.toString());
--- End diff --
I agree with your point. I am also not confortable with the place I put this test, but just to explain myself:
I've chosen to improve this already existing test because its name is "testErrorPolicies" and the fix was exactly on the errorPolicies behaviour. Moreover, the already existing test, tests the injection of LOST after 100% of the session timeout has elapsed, I've just copy-pasted some lines, and set to inject lost after 30% of sessionTimeout.
In my opinion, if we move the lines I added from here to another test class, we should also move the whole already existing testErrorPolicies method.
What do you think?
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179022050
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -253,6 +253,7 @@ private void processEvents()
{
int lastNegotiatedSessionTimeoutMs = client.getZookeeperClient().getLastNegotiatedSessionTimeoutMs();
int useSessionTimeoutMs = (lastNegotiatedSessionTimeoutMs > 0) ? lastNegotiatedSessionTimeoutMs : sessionTimeoutMs;
+ useSessionTimeoutMs = sessionExpirationPercent > 0 && startOfSuspendedEpoch != 0 ? (useSessionTimeoutMs * sessionExpirationPercent) / 100 : useSessionTimeoutMs;
--- End diff --
Yeah, you're probably right. If we change to volatile there will be quite a few more changes.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
To Curator, don't they all look the same? Ultimately, Curator will just get a Disconnected event?
Given that we can't tell the difference between them, I think that we just have to document this as a limitation (tech note and in the relevant classes?) and maybe send out an email on the users email list.
I think that we should ultimately just leave the other handling as is. Different % values can be passed to StandardConnectionHandlingPolicy to achieve the same thing as an AggressiveConnectionHandlingPolicy just by passing in a value of 33 yes?
This PR is still relevant though and should be merged.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/curator/pull/262
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/262
> -If the connection between client and server is lost a disconnected event is received essentially immediately.
> -If a heart beat is missed it takes 2/3 of the session timeout.
Thinking more about this... There are three scenarios:
1. The internal ZK client detects a missed heartbeat, closes the connection and sends `Event.KeeperState.Disconnected` (note: this is done by ClientCnxn.java in the SendThread class's run() method - the huge catch handler).
2. The server detects a missed heartbeat and closes the connection which causes the client to get a closed connection and most likely a `SocketException` (which is handled in that same catch clause)
3. The connection fails for other reasons (TCP errors, server crashes, etc.).
Unfortunately, cases 2 and 3 look _exactly the same_ to the client. In case 2, the client should ideally act as if 2/3 of the session have elapsed. In case 3, the client should act as if none of the session has elapsed. Worse still, if the entire ZK cluster is down, it could conceivably come back up and clients won't lose their sessions at all because "time" in ZK is based on the leader's start time (I still think there's value in killing the session on the client side anyway as clients could be left hanging interminably).
So, even if we can know the client side heartbeat miss (case 1), we have no way of mitigating cases 2/3. I'm not sure what to do. Maybe leave `StandardConnectionHandlingPolicy` as is and add a new, optional policy, `AggressiveConnectionHandlingPolicy` that always assumes connection loss is due to missed heartbeats? Or, possibly, just do nothing and leave things as they are?
Thoughts?
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179654000
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -340,4 +338,12 @@ private void setCurrentConnectionState(ConnectionState newConnectionState)
currentConnectionState = newConnectionState;
startOfSuspendedEpoch = (currentConnectionState == ConnectionState.SUSPENDED) ? System.currentTimeMillis() : 0;
}
+
+ private synchronized int getUseSessionTimeoutMs() {
--- End diff --
I've made it volatile now.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179010977
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -253,6 +253,7 @@ private void processEvents()
{
int lastNegotiatedSessionTimeoutMs = client.getZookeeperClient().getLastNegotiatedSessionTimeoutMs();
int useSessionTimeoutMs = (lastNegotiatedSessionTimeoutMs > 0) ? lastNegotiatedSessionTimeoutMs : sessionTimeoutMs;
+ useSessionTimeoutMs = sessionExpirationPercent > 0 && startOfSuspendedEpoch != 0 ? (useSessionTimeoutMs * sessionExpirationPercent) / 100 : useSessionTimeoutMs;
--- End diff --
I agree that make startOfSuspendedEpoch vollatile would be cleaner, but given that all other points accessing startOfSuspendedEpoch are already synchronized, I think that synchronize also this point would be more consistent with the rest of the code. Just an oppinion.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/262
I'm thinking we change `StandardConnectionHandlingPolicy`. It should be 33 though right? By the time we get the Disconnected 2/3 of a session have elapsed.
However, this could affect existing clients badly. So, probably the best thing is to make the value configurable via command line with the default being 33. So, something like:
```
public class StandardConnectionHandlingPolicy implements ConnectionHandlingPolicy
{
private static final int default_expiration_percent = Integer.getInteger("curator-connection-expiration-percent", 33);
private final int expirationPercent;
public StandardConnectionHandlingPolicy()
{
this(default_expiration_percent);
}
}
```
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179652781
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -340,4 +338,12 @@ private void setCurrentConnectionState(ConnectionState newConnectionState)
currentConnectionState = newConnectionState;
startOfSuspendedEpoch = (currentConnectionState == ConnectionState.SUSPENDED) ? System.currentTimeMillis() : 0;
}
+
+ private synchronized int getUseSessionTimeoutMs() {
--- End diff --
I think volatile is cleaner. I don't see any read/increments so it doesn't need to be AtomicLong either.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
I'm conflicted about the LOST injection stuff. I guess all we can do is err on the side of caution and leave it as is. That will cover what is presumably the most common case, where the connection to ZK is lost rather than the case this JIRA covers where the connection is still up but there's no heart beat.
While we will never be able to exactly match the timing on the server side, I think that we should strive to have a reasonable approximation of it. Not being able to tell when the last heart beat was successfully sent to the server is problematic.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179653281
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -340,4 +338,12 @@ private void setCurrentConnectionState(ConnectionState newConnectionState)
currentConnectionState = newConnectionState;
startOfSuspendedEpoch = (currentConnectionState == ConnectionState.SUSPENDED) ? System.currentTimeMillis() : 0;
}
+
+ private synchronized int getUseSessionTimeoutMs() {
--- End diff --
Perfect.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by arrodrigues <gi...@git.apache.org>.
Github user arrodrigues commented on the issue:
https://github.com/apache/curator/pull/262
@cammckenzie It is on 2/3 of session timeout.
I'd like to remember this will happen just in the scenario that I described in the issue description, in many other network issues the suspended event would came immediatly.
Beyond the 33%, shouldn't we consider adding an margin of error, like using 30%, 25% ? Because besides the 33% of session timeout, we could also have some delay to execute the leadership relinquish. Furthermore, I think ZK's ticktime, that generally is 2sec, also can potentially adds more delay to the formula.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/262
The new Tech Note: https://cwiki.apache.org/confluence/display/CURATOR/TN14 - I'd appreciate reviews.
---
[GitHub] curator pull request #262: [CURATOR-460] Timed tolerance for connection susp...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/262#discussion_r179651175
--- Diff: curator-framework/src/main/java/org/apache/curator/framework/state/ConnectionStateManager.java ---
@@ -340,4 +338,12 @@ private void setCurrentConnectionState(ConnectionState newConnectionState)
currentConnectionState = newConnectionState;
startOfSuspendedEpoch = (currentConnectionState == ConnectionState.SUSPENDED) ? System.currentTimeMillis() : 0;
}
+
+ private synchronized int getUseSessionTimeoutMs() {
--- End diff --
Looks like the comment on `startOfSuspendedEpoch` "guarded by sync" is a lie. `getUseSessionTimeoutMs()` doesn't need to be synchronized if we make `startOfSuspendedEpoch` volatile.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by Randgalt <gi...@git.apache.org>.
Github user Randgalt commented on the issue:
https://github.com/apache/curator/pull/262
Yeah, I agree @cammckenzie. Let's merge what we have here and I'll write up a Tech Note.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
The changes look good to me, thanks @arrodrigues
@Randgalt , I think the approach you're suggesting for setting up the default session timeout stuff looks OK. I thought that the suspended event came at 1/3 of the session timeout, which is why I was suggesting 66%. I will need to double check. If it's 2/3 of session timeout, then yes, 33%
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Posted by cammckenzie <gi...@git.apache.org>.
Github user cammckenzie commented on the issue:
https://github.com/apache/curator/pull/262
That's a good point @arrodrigues
I don't think we can tell the difference between Disconnected event from ZK due to loss of the underlying socket and a Disconected event due to no heartbeats from the server for 2/3 of session timeout, which, from memory is what you were describing at the start of this conversation long ago.
With a bit of hacking, I believe that could extended the Zookeeper class to allow Curator to get access to the ClientCnxn object and from that, the underlying socket. I don't know if this is the right approach though, it will certainly couple Curator more tightly to ZK.
Thoughts @Randgalt ?
---