[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/303#discussion_r251685066
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -48,19 +50,21 @@
public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2,
BackgroundOperation, ErrorListenerPathAndBytesable
{
+private final Logger log = LoggerFactory.getLogger(getClass());
private final CuratorFrameworkImpl client;
private CreateMode createMode;
private Backgrounding backgrounding;
private boolean createParentsIfNeeded;
private boolean createParentsAsContainers;
-private boolean doProtected;
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
-private String protectedId;
private ACLing acling;
private Stat storingStat;
private long ttl;
+private boolean doProtected;
+private String protectedId;
+private long initialSessionId;
--- End diff --
protectedEphemeralSessionID? And then maybe move the initialisation check
so that it only occurs if Curator is actually creating a protected ephemeral
node, rather than always initialising on the first forPath() call?
---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/303#discussion_r251672915
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -48,19 +50,21 @@
public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2,
BackgroundOperation, ErrorListenerPathAndBytesable
{
+private final Logger log = LoggerFactory.getLogger(getClass());
private final CuratorFrameworkImpl client;
private CreateMode createMode;
private Backgrounding backgrounding;
private boolean createParentsIfNeeded;
private boolean createParentsAsContainers;
-private boolean doProtected;
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
-private String protectedId;
private ACLing acling;
private Stat storingStat;
private long ttl;
+private boolean doProtected;
+private String protectedId;
+private long initialSessionId;
--- End diff --
I think that maybe the initialSessionId should have a different name as
it's a bit misleading. It's not really the initialSessionId as it gets updated
during the edge case for handling protected ephemeral nodes. Really, it's the
session ID at the time of creation for a protected ephemeral node isn't it?
Perhaps it should be named accordingly and only initialised for the case where
it will be needed?
---
[GitHub] curator issue #257: CURATOR-458: Fix a bug spotted by error-prone compiler
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/257 Any thoughts on this @Randgalt . I've had a look at it, and the changes seem fine, but equally, they're not adding a great deal (other than stopping some error prone complaints). ---
[GitHub] curator issue #297: [CURATOR-495] Fixes race in many Curator recipes which c...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/297 I have just run the tests and everything passed, which is a bit of a novelty! Still failed at the end with that surefire issue. I think that this is good to merge though. Nice work! ---
[GitHub] curator pull request #297: [CURATOR-495] Fixes race in many Curator recipes ...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/297#discussion_r242005321
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java
---
@@ -474,6 +477,28 @@ public Builder schemaSet(SchemaSet schemaSet)
return this;
}
+/**
+ * Curator (and user) recipes will use this executor to call
notifyAll
+ * and other blocking calls that might normally block ZooKeeper's
event thread.
+ * By default, an executor is allocated internally using the
provided (or default)
+ * {@link #threadFactory(java.util.concurrent.ThreadFactory)}. Use
this method
+ * to set a custom executor.
+ *
+ * @param runSafeService executor to use for calls to notifyAll
from Watcher callbacks etc
+ * @return this
+ * @since 4.1.0
+ */
+public Builder runSafeService(Executor runSafeService)
+{
+this.runSafeService = runSafeService;
+return null;
--- End diff --
This should return this rather than null
---
[GitHub] curator issue #278: [CURATOR-477] added support for turning off zk watches
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/278 I will merge this shortly, thanks @ramaraochavali and @dragonsinth ---
[GitHub] curator issue #294: [CURATOR-479] fixed CachedModeledFrameworkImpl children
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/294 Looks good to me. ---
[GitHub] curator issue #282: CURATOR-487 Make GzipCompressionProvider to recycle Defl...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/282 Thanks for the PR @alexbrasetvik I will merge this shortly. ---
[GitHub] curator issue #280: CURATOR-481 Remove jackson-mapper-asl-version and update...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/280 @Max-Pudov @matobet Guys, any thoughts about backward compatibility issues? ---
[GitHub] curator issue #285: [CURATOR-468] LeaderSelector interruption causing spurio...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/285 Looks ok to me. ---
[GitHub] curator issue #292: [CURATOR-405] reset startOfSuspendedEpoch when session e...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/292 Looks good to me. ---
[GitHub] curator issue #283: CURATOR-472 - fix deadlocks during tests as well as TTL ...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/283 Looks good to me. I pushed a fixed to the TestTtlNodes unit test is it was consistently failing for me. ---
[GitHub] curator issue #281: [CURATOR-483] Fix path used when re-creating sequential ...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/281 Thanks for the fix @njhill ---
[GitHub] curator issue #281: [CURATOR-483] Fix path used when re-creating sequential ...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/281 Yep, I've been meaning to merge, just having issues running some unit tests (which are unrelated to this PR). I will try and merge it today. ---
[GitHub] curator issue #243: [Fix] Curator-444
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/243 This is just done for efficiency. Each client involved in the leader election only cares when the child before it disconnects, because that implies that it should now become leader. If any of the other children disconnect, the client doesn't care because it doesn't affect whether it should become leader or not. ---
[GitHub] curator pull request #281: [CURATOR-483] Fix path used when re-creating sequ...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/281#discussion_r233280744
--- Diff:
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java
---
@@ -445,7 +445,8 @@ private void createNode()
try
{
String existingPath = nodePath.get();
-String createPath = (existingPath != null && !useProtection) ?
existingPath : basePath;
+String createPath = existingPath == null || (useProtection &&
!isSequential(mode)) ? basePath
+: (useProtection ? basePath +
existingPath.substring(existingPath.length()-10) : existingPath);
--- End diff --
Good question @njhill I don't really have a super strong preference, but
I'd prefer that the constant defining the size of the sequential bit is not in
the PersisteNode class. Perhaps just have a method in ZKPaths that extracts the
last 10 digits without any checks. We can make it more defensive down the track
if it gets used elsewhere.
I don't think that efficiency is a huge concern here as it's only going to
happen when the node is created.
---
[GitHub] curator pull request #281: [CURATOR-483] Fix path used when re-creating sequ...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/281#discussion_r233227009
--- Diff:
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java
---
@@ -445,7 +445,8 @@ private void createNode()
try
{
String existingPath = nodePath.get();
-String createPath = (existingPath != null && !useProtection) ?
existingPath : basePath;
+String createPath = existingPath == null || (useProtection &&
!isSequential(mode)) ? basePath
+: (useProtection ? basePath +
existingPath.substring(existingPath.length()-10) : existingPath);
--- End diff --
Thanks @Randgalt, I figured that there was a good place to put this, but
couldn't remember where it should be. @njhill can you refactor the sequential
number extraction into org.apache.curator.utils.ZKPaths class? Or, if you don't
have time, let me know and I'll get it done.
---
[GitHub] curator issue #280: CURATOR-481 Remove jackson-mapper-asl-version and update...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/280 It looks OK to me, but my knowledge of this section of Curator is very limited. @Randgalt do you have any thoughts on this? If not, I'll merge. ---
[GitHub] curator pull request #281: [CURATOR-483] Fix path used when re-creating sequ...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/281#discussion_r231379350
--- Diff:
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java
---
@@ -444,8 +447,19 @@ private void createNode()
try
{
-String existingPath = nodePath.get();
-String createPath = (existingPath != null && !useProtection) ?
existingPath : basePath;
+String existingPath = nodePath.get(), createPath;
+if ( existingPath != null && !useProtection )
+{
+createPath = existingPath;
+}
+else
+{
+createPath = basePath;
+if ( useProtection && mode.isSequential() )
+{
+createPath +=
existingPath.substring(existingPath.length()-SEQUENTIAL_SUFFIX_DIGITS);
--- End diff --
This logic is now incorrect. When it's first run the existingPath will be
null.
---
[GitHub] curator pull request #281: [CURATOR-483] Fix path used when re-creating sequ...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/281#discussion_r231322157
--- Diff:
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentNode.java
---
@@ -445,7 +445,8 @@ private void createNode()
try
{
String existingPath = nodePath.get();
-String createPath = (existingPath != null && !useProtection) ?
existingPath : basePath;
+String createPath = existingPath == null || (useProtection &&
!isSequential(mode)) ? basePath
+: (useProtection ? basePath +
existingPath.substring(existingPath.length()-10) : existingPath);
--- End diff --
I think that this would be cleaner as some if statements. Also, the 10
magic number is not ideal, but I don't believe that we have a constant defining
the length of the sequential part of the node name anywhere. Not sure where the
best place would be, perhaps just in the CuratorFramework interface. Thoughts
@Randgalt ?
---
[GitHub] curator issue #281: [CURATOR-483] Fix path used when re-creating sequential ...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/281 @njhill Thanks for the PR. Would it be possible to provide a unit test to reproduce the problem and verify the fix? cheers ---
[GitHub] curator issue #275: [CURATOR-476] PathChildrenCache should check resultCode=...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/275 Thanks for the PR. Looks good to me. I will merge to master shortly. ---
[GitHub] curator issue #263: [CURATOR-462] InterProcessSemaphoreV2 leaves orphaned le...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/263 Looks good to me, will merge shortly. ---
[GitHub] curator pull request #257: CURATOR-458: Use error-prone compiler
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/257#discussion_r183254951
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/schema/Schema.java
---
@@ -105,7 +105,8 @@ public static SchemaBuilder builderForRecipe(String
parentPath)
Schema(String name, Pattern pathRegex, String path, String
documentation, SchemaValidator schemaValidator, Allowance ephemeral, Allowance
sequential, Allowance watched, boolean canBeDeleted, Map
metadata)
{
-Preconditions.checkNotNull((pathRegex != null) || (path != null),
"pathRegex and path cannot both be null");
+Preconditions.checkNotNull(pathRegex, "pathRegex cannot both be
null");
+Preconditions.checkNotNull(path, "path cannot both be null");
this.pathRegex = pathRegex;
--- End diff --
The existing check here seems like it was incorrect, as it would have
returned a boolean which would never be null. This change will fix it, but
there error messages are now incorrect. They should just be "pathRegex cannot
be null" and "path cannot be null" respectively.
---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/262 Ok, will merge this now. Thanks @arrodrigues ! ---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
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 issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
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 issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
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 issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
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 ? ---
[GitHub] curator issue #262: [CURATOR-460] Timed tolerance for connection suspended l...
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...
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 pull request #262: [CURATOR-460] Timed tolerance for connection susp...
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...
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 pull request #262: [CURATOR-460] Timed tolerance for connection susp...
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...
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 #252: [CURATOR-455] Curator's EnsembleProvider.getConnectionSt...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/252 Looks good to me. ---
[GitHub] curator pull request #252: [CURATOR-455] Curator's EnsembleProvider.getConne...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/252#discussion_r169528858
--- Diff:
curator-framework/src/test/java/org/apache/curator/framework/ensemble/TestEnsembleProvider.java
---
@@ -0,0 +1,162 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.ensemble;
+
+import org.apache.curator.ensemble.EnsembleProvider;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.framework.state.ConnectionState;
+import org.apache.curator.framework.state.ConnectionStateListener;
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.BaseClassForTests;
+import org.apache.curator.test.TestingServer;
+import org.apache.curator.test.Timing;
+import org.apache.curator.utils.CloseableUtils;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
+
+public class TestEnsembleProvider extends BaseClassForTests
+{
+private final Timing timing = new Timing();
+
+@Test
+public void testBasic()
+{
+Semaphore counter = new Semaphore(0);
+final CuratorFramework client = newClient(counter);
+try
+{
+client.start();
+Assert.assertTrue(timing.acquireSemaphore(counter));
+}
+finally
+{
+CloseableUtils.closeQuietly(client);
+}
+}
+
+@Test
+public void testAfterSessionExpiration() throws Exception
+{
+TestingServer oldServer = server;
+Semaphore counter = new Semaphore(0);
+final CuratorFramework client = newClient(counter);
+try
+{
+client.start();
+
+final CountDownLatch connectedLatch = new CountDownLatch(1);
+final CountDownLatch lostLatch = new CountDownLatch(1);
+final CountDownLatch reconnectedLatch = new CountDownLatch(1);
+ConnectionStateListener listener = new
ConnectionStateListener()
+{
+@Override
+public void stateChanged(CuratorFramework client,
ConnectionState newState)
+{
+if ( newState == ConnectionState.CONNECTED )
+{
+connectedLatch.countDown();
+}
+if ( newState == ConnectionState.LOST )
+{
+lostLatch.countDown();
+}
+if ( newState == ConnectionState.RECONNECTED )
+{
+reconnectedLatch.countDown();
+}
+}
+};
+client.getConnectionStateListenable().addListener(listener);
--- End diff --
Is there a potential race condition here? Can't the connected event come
before the listener is added as the client is started prior to the listener
being added?
---
[GitHub] curator issue #249: [CURATOR-388] PathChildrenCache stops working if contain...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/249 I think this looks OK. ---
[GitHub] curator issue #248: [CURATOR-446] Update docs to document how to use the cur...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/248 Looks ok to me. ---
[GitHub] curator issue #243: [Fix] Curator-444
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/243 Thanks! ---
[GitHub] curator pull request #243: [Fix] Curator-444
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/243#discussion_r154539096 --- Diff: curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/testing/DummyLeaderLatch.java --- @@ -0,0 +1,189 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.curator.framework.recipes.leader.testing; + +import static org.apache.curator.framework.recipes.leader.LeaderLatch.CloseMode.NOTIFY_LEADER; + +import java.io.IOException; +import java.util.LinkedList; +import java.util.List; +import java.util.concurrent.atomic.AtomicBoolean; + +import javax.xml.ws.Holder; --- End diff -- Unused import? ---
[GitHub] curator pull request #243: [Fix] Curator-444
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/243#discussion_r154539053
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatchIsolatedZookeeper.java
---
@@ -0,0 +1,224 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.recipes.leader;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.net.SocketException;
+
+import
org.apache.curator.framework.recipes.leader.testing.DummyLeaderLatch;
+import org.apache.curator.test.TestingCluster;
+import org.apache.curator.test.TestingZooKeeperServer;
+import org.apache.zookeeper.server.quorum.Leader;
+import org.apache.zookeeper.server.quorum.LearnerHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+/** Tests issue: CURATOR-444: Zookeeper node is isolated from other
zookeepers but not from curator
+ * temporally */
+
+
+public class TestLeaderLatchIsolatedZookeeper {
+
+ private static final Logger logger =
LoggerFactory.getLogger(TestLeaderLatchIsolatedZookeeper.class);
+ private static final int CONNECTION_TIMEOUT_MS = 15000;
+ private static final int SESSION_TIMEOUT_MS = 1;
+
+ private TestingCluster cluster;
+ private DummyLeaderLatch firstClient;
+ private DummyLeaderLatch secondClient;
+
+ @BeforeMethod
+ public void beforeMethod() throws Exception {
+ cluster = new TestingCluster(3);
+ cluster.start();
+ firstClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "First");
+ secondClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "Second");
+ firstClient.startAndAwaitElection();
+ secondClient.startAndAwaitElection();
+ logger.info("Session: " + SESSION_TIMEOUT_MS + " connection " +
CONNECTION_TIMEOUT_MS );
+ }
+
+ @AfterMethod
+ public void afterMethod() throws IOException {
+ firstClient.stop();
+ secondClient.stop();
+ cluster.close();
+ DummyLeaderLatch.resetHistory();
+ }
+
+ @Test
+ public void testThatStartsWithOnlyOneLeader() throws Exception {
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ @Test
+ public void testThatStartCoherent() throws Exception {
+ assertEquals(firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals(secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ }
+
+ @Test(invocationCount = 10)
+ public void testThatResistsNetworkGlitches() throws Exception {
+
+ blockLeaderListeningForSomeTime(SESSION_TIMEOUT_MS);
+
+ Thread.sleep(SESSION_TIMEOUT_MS * 3);
+
+ assertTrue(isHistoryValid(), "History is not valid: " +
DummyLeaderLatch.getEventHistory());
+
+ assertEquals( firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals( secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ private void blockLeaderListeningForSomeTime(long milliseconds) throws
InterruptedException {
[GitHub] curator pull request #243: [Fix] Curator-444
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/243#discussion_r154539158
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatchIsolatedZookeeper.java
---
@@ -0,0 +1,224 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.recipes.leader;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.net.SocketException;
+
+import
org.apache.curator.framework.recipes.leader.testing.DummyLeaderLatch;
+import org.apache.curator.test.TestingCluster;
+import org.apache.curator.test.TestingZooKeeperServer;
+import org.apache.zookeeper.server.quorum.Leader;
+import org.apache.zookeeper.server.quorum.LearnerHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+/** Tests issue: CURATOR-444: Zookeeper node is isolated from other
zookeepers but not from curator
+ * temporally */
+
+
+public class TestLeaderLatchIsolatedZookeeper {
+
+ private static final Logger logger =
LoggerFactory.getLogger(TestLeaderLatchIsolatedZookeeper.class);
+ private static final int CONNECTION_TIMEOUT_MS = 15000;
+ private static final int SESSION_TIMEOUT_MS = 1;
+
+ private TestingCluster cluster;
+ private DummyLeaderLatch firstClient;
+ private DummyLeaderLatch secondClient;
+
+ @BeforeMethod
+ public void beforeMethod() throws Exception {
+ cluster = new TestingCluster(3);
+ cluster.start();
+ firstClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "First");
+ secondClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "Second");
+ firstClient.startAndAwaitElection();
+ secondClient.startAndAwaitElection();
+ logger.info("Session: " + SESSION_TIMEOUT_MS + " connection " +
CONNECTION_TIMEOUT_MS );
+ }
+
+ @AfterMethod
+ public void afterMethod() throws IOException {
+ firstClient.stop();
+ secondClient.stop();
+ cluster.close();
+ DummyLeaderLatch.resetHistory();
+ }
+
+ @Test
+ public void testThatStartsWithOnlyOneLeader() throws Exception {
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ @Test
+ public void testThatStartCoherent() throws Exception {
+ assertEquals(firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals(secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ }
+
+ @Test(invocationCount = 10)
+ public void testThatResistsNetworkGlitches() throws Exception {
--- End diff --
Is it possible to make the test more reliable? Running it 10 times at 30
seconds an invocation increases our regression run by 5 minutes, which isn't
ideal.
---
[GitHub] curator pull request #243: [Fix] Curator-444
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/243#discussion_r154539068
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatchIsolatedZookeeper.java
---
@@ -0,0 +1,224 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.recipes.leader;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.net.SocketException;
+
+import
org.apache.curator.framework.recipes.leader.testing.DummyLeaderLatch;
+import org.apache.curator.test.TestingCluster;
+import org.apache.curator.test.TestingZooKeeperServer;
+import org.apache.zookeeper.server.quorum.Leader;
+import org.apache.zookeeper.server.quorum.LearnerHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+/** Tests issue: CURATOR-444: Zookeeper node is isolated from other
zookeepers but not from curator
+ * temporally */
+
+
+public class TestLeaderLatchIsolatedZookeeper {
+
+ private static final Logger logger =
LoggerFactory.getLogger(TestLeaderLatchIsolatedZookeeper.class);
+ private static final int CONNECTION_TIMEOUT_MS = 15000;
+ private static final int SESSION_TIMEOUT_MS = 1;
+
+ private TestingCluster cluster;
+ private DummyLeaderLatch firstClient;
+ private DummyLeaderLatch secondClient;
+
+ @BeforeMethod
+ public void beforeMethod() throws Exception {
+ cluster = new TestingCluster(3);
+ cluster.start();
+ firstClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "First");
+ secondClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "Second");
+ firstClient.startAndAwaitElection();
+ secondClient.startAndAwaitElection();
+ logger.info("Session: " + SESSION_TIMEOUT_MS + " connection " +
CONNECTION_TIMEOUT_MS );
+ }
+
+ @AfterMethod
+ public void afterMethod() throws IOException {
+ firstClient.stop();
+ secondClient.stop();
+ cluster.close();
+ DummyLeaderLatch.resetHistory();
+ }
+
+ @Test
+ public void testThatStartsWithOnlyOneLeader() throws Exception {
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ @Test
+ public void testThatStartCoherent() throws Exception {
+ assertEquals(firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals(secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ }
+
+ @Test(invocationCount = 10)
+ public void testThatResistsNetworkGlitches() throws Exception {
+
+ blockLeaderListeningForSomeTime(SESSION_TIMEOUT_MS);
+
+ Thread.sleep(SESSION_TIMEOUT_MS * 3);
+
+ assertTrue(isHistoryValid(), "History is not valid: " +
DummyLeaderLatch.getEventHistory());
+
+ assertEquals( firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals( secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ private void blockLeaderListeningForSomeTime(long milliseconds) throws
InterruptedException {
[GitHub] curator pull request #243: [Fix] Curator-444
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/243#discussion_r154539070
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatchIsolatedZookeeper.java
---
@@ -0,0 +1,224 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.recipes.leader;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.net.SocketException;
+
+import
org.apache.curator.framework.recipes.leader.testing.DummyLeaderLatch;
+import org.apache.curator.test.TestingCluster;
+import org.apache.curator.test.TestingZooKeeperServer;
+import org.apache.zookeeper.server.quorum.Leader;
+import org.apache.zookeeper.server.quorum.LearnerHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+/** Tests issue: CURATOR-444: Zookeeper node is isolated from other
zookeepers but not from curator
+ * temporally */
+
+
+public class TestLeaderLatchIsolatedZookeeper {
+
+ private static final Logger logger =
LoggerFactory.getLogger(TestLeaderLatchIsolatedZookeeper.class);
+ private static final int CONNECTION_TIMEOUT_MS = 15000;
+ private static final int SESSION_TIMEOUT_MS = 1;
+
+ private TestingCluster cluster;
+ private DummyLeaderLatch firstClient;
+ private DummyLeaderLatch secondClient;
+
+ @BeforeMethod
+ public void beforeMethod() throws Exception {
+ cluster = new TestingCluster(3);
+ cluster.start();
+ firstClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "First");
+ secondClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "Second");
+ firstClient.startAndAwaitElection();
+ secondClient.startAndAwaitElection();
+ logger.info("Session: " + SESSION_TIMEOUT_MS + " connection " +
CONNECTION_TIMEOUT_MS );
+ }
+
+ @AfterMethod
+ public void afterMethod() throws IOException {
+ firstClient.stop();
+ secondClient.stop();
+ cluster.close();
+ DummyLeaderLatch.resetHistory();
+ }
+
+ @Test
+ public void testThatStartsWithOnlyOneLeader() throws Exception {
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ @Test
+ public void testThatStartCoherent() throws Exception {
+ assertEquals(firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals(secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ }
+
+ @Test(invocationCount = 10)
+ public void testThatResistsNetworkGlitches() throws Exception {
+
+ blockLeaderListeningForSomeTime(SESSION_TIMEOUT_MS);
+
+ Thread.sleep(SESSION_TIMEOUT_MS * 3);
+
+ assertTrue(isHistoryValid(), "History is not valid: " +
DummyLeaderLatch.getEventHistory());
+
+ assertEquals( firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals( secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ private void blockLeaderListeningForSomeTime(long milliseconds) throws
InterruptedException {
[GitHub] curator pull request #243: [Fix] Curator-444
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/243#discussion_r154539071
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderLatchIsolatedZookeeper.java
---
@@ -0,0 +1,224 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.recipes.leader;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+import static org.testng.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.Socket;
+import java.net.SocketException;
+
+import
org.apache.curator.framework.recipes.leader.testing.DummyLeaderLatch;
+import org.apache.curator.test.TestingCluster;
+import org.apache.curator.test.TestingZooKeeperServer;
+import org.apache.zookeeper.server.quorum.Leader;
+import org.apache.zookeeper.server.quorum.LearnerHandler;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+/** Tests issue: CURATOR-444: Zookeeper node is isolated from other
zookeepers but not from curator
+ * temporally */
+
+
+public class TestLeaderLatchIsolatedZookeeper {
+
+ private static final Logger logger =
LoggerFactory.getLogger(TestLeaderLatchIsolatedZookeeper.class);
+ private static final int CONNECTION_TIMEOUT_MS = 15000;
+ private static final int SESSION_TIMEOUT_MS = 1;
+
+ private TestingCluster cluster;
+ private DummyLeaderLatch firstClient;
+ private DummyLeaderLatch secondClient;
+
+ @BeforeMethod
+ public void beforeMethod() throws Exception {
+ cluster = new TestingCluster(3);
+ cluster.start();
+ firstClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "First");
+ secondClient = new DummyLeaderLatch(cluster.getConnectString(),
SESSION_TIMEOUT_MS, CONNECTION_TIMEOUT_MS, "Second");
+ firstClient.startAndAwaitElection();
+ secondClient.startAndAwaitElection();
+ logger.info("Session: " + SESSION_TIMEOUT_MS + " connection " +
CONNECTION_TIMEOUT_MS );
+ }
+
+ @AfterMethod
+ public void afterMethod() throws IOException {
+ firstClient.stop();
+ secondClient.stop();
+ cluster.close();
+ DummyLeaderLatch.resetHistory();
+ }
+
+ @Test
+ public void testThatStartsWithOnlyOneLeader() throws Exception {
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ @Test
+ public void testThatStartCoherent() throws Exception {
+ assertEquals(firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals(secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ }
+
+ @Test(invocationCount = 10)
+ public void testThatResistsNetworkGlitches() throws Exception {
+
+ blockLeaderListeningForSomeTime(SESSION_TIMEOUT_MS);
+
+ Thread.sleep(SESSION_TIMEOUT_MS * 3);
+
+ assertTrue(isHistoryValid(), "History is not valid: " +
DummyLeaderLatch.getEventHistory());
+
+ assertEquals( firstClient.isLeaderAccordingToLatch(),
firstClient.isLeaderAccordingToEvents());
+ assertEquals( secondClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToEvents());
+ assertNotEquals(firstClient.isLeaderAccordingToLatch(),
secondClient.isLeaderAccordingToLatch());
+ }
+
+ private void blockLeaderListeningForSomeTime(long milliseconds) throws
InterruptedException {
[GitHub] curator issue #240: CURATOR-438 Replace Deprecated Methods
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/240 Looks ok to me. ---
[GitHub] curator issue #238: [CURATOR-436] getSortedChildren() should ignore NoNode e...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/238 Looks good to me. ---
[GitHub] curator pull request #236: CURATOR-431 - Fixed stat population during create
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/236#discussion_r138482206
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -1212,7 +1212,21 @@ public String call() throws Exception
{
if ( setDataIfExists )
{
-client.getZooKeeper().setData(path,
data, setDataIfExistsVersion);
+Stat setStat =
client.getZooKeeper().setData(path, data, setDataIfExistsVersion);
+if(storingStat != null)
+{
+
storingStat.setAversion(setStat.getAversion());
--- End diff --
Thanks, there was another place in CreateBuilderImpl doing the same thing,
and I've replaced that with DataTree#copyStat() also.
---
[GitHub] curator pull request #236: CURATOR-431 - Fixed stat population during create
GitHub user cammckenzie opened a pull request: https://github.com/apache/curator/pull/236 CURATOR-431 - Fixed stat population during create -The stat object was not being populated if the create failed due to the node already existing. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/curator CURATOR-431 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/curator/pull/236.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 #236 commit 931f1de5c4f72b71e96ea2071add297194e053bd Author: Cam McKenzie Date: 2017-09-11T22:58:47Z CURATOR-431 - Fixed stat population during create -The stat object was not being populated if the create failed due to the node already existing. ---
[GitHub] curator pull request #223: [CURATOR-362] Use provided ACL when creating pare...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/223#discussion_r123912201
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/ExistsBuilderImpl.java
---
@@ -185,7 +185,7 @@ public Stat forPath(String path) throws Exception
OperationAndData operationAndData = new
OperationAndData(this, path, backgrounding.getCallback(), null,
backgrounding.getContext(), watching);
if ( createParentContainersIfNeeded || createParentsIfNeeded )
{
-CreateBuilderImpl.backgroundCreateParentsThenNode(client,
operationAndData, operationAndData.getData(), backgrounding,
createParentContainersIfNeeded);
+CreateBuilderImpl.backgroundCreateParentsThenNode(client,
operationAndData, operationAndData.getData(), backgrounding,
client.getAclProvider(), createParentContainersIfNeeded);
--- End diff --
Should the ExistsBuilder have the ability to set the ACL provider for the
parent containers as well, given that the we provide the option of creating
parent containers?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #221: [CURATOR-411] Fix various testing issues
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/221 With that disabled, all tests passed for me on the second run. On the first run, one failed due to not all watchers being removed, but I lost the output and on subsequent runs it has worked each time. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #208: [CURATOR-394] UnrecognizedPropertyException: "enabled" i...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/208 I think that what we've got is probably the best compromise we're going to get. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #223: [CURATOR-362] Use provided ACL when creating parent dire...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/223 Sounds good to me, thanks @szekizoli --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #223: [CURATOR-362] Use provided ACL when creating parent dire...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/223 I agree that this is now a 'feature' that the parents don't have the ACL set. Perhaps we need to introduce another option to set the ACL on the parents? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #221: [CURATOR-411] Fix various testing issues
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/221 I had a bit more of a look at the failing test and see the following being logged when the client.reconfig() call occurs. I'm not sure if these are issues or not, but they certainly seem to be. Also, I noticed that the reconfig() call to Zookeeper that Curator is using is deprecated. ERROR org.apache.zookeeper.server.NIOServerCnxnFactory Error reconfiguring client port to /0.0.0.0:39762 Address already in use [LearnerHandler-/127.0.0.1:52184] ERROR org.apache.zookeeper.server.quorum.QuorumCnxManager Exception while listening [/0.0.0.0:34491] java.net.BindException: Address already in use (Bind failed) at java.net.PlainSocketImpl.socketBind(Native Method) at java.net.AbstractPlainSocketImpl.bind(AbstractPlainSocketImpl.java:376) at java.net.ServerSocket.bind(ServerSocket.java:376) at java.net.ServerSocket.bind(ServerSocket.java:330) at org.apache.zookeeper.server.quorum.QuorumCnxManager$Listener.run(QuorumCnxManager.java:638) ERROR org.apache.zookeeper.server.NIOServerCnxnFactory Error reconfiguring client port to /0.0.0.0:35059 Address already in use [QuorumPeer[myid=2](plain=/127.0.0.1:35059)(secure=disabled)] ERROR org.apache.zookeeper.server.NIOServerCnxnFactory Error reconfiguring client port to /0.0.0.0:33568 Address already in use [QuorumPeer[myid=1](plain=/127.0.0.1:33568)(secure=disabled)] ERROR org.apache.zookeeper.server.quorum.LearnerHandler Unexpected exception causing shutdown while sock still open [LearnerHandler-/127.0.0.1:52184] java.io.EOFException at java.io.DataInputStream.readInt(DataInputStream.java:392) at org.apache.jute.BinaryInputArchive.readInt(BinaryInputArchive.java:63) at org.apache.zookeeper.server.quorum.QuorumPacket.deserialize(QuorumPacket.java:83) at org.apache.jute.BinaryInputArchive.readRecord(BinaryInputArchive.java:99) at org.apache.zookeeper.server.quorum.LearnerHandler.run(LearnerHandler.java:515) ERROR org.apache.zookeeper.server.quorum.LearnerHandler Unexpected exception causing shutdown while sock still open [LearnerHandler-/127.0.0.1:52186] java.io.EOFException at java.io.DataInputStream.readInt(DataInputStream.java:392) at org.apache.jute.BinaryInputArchive.readInt(BinaryInputArchive.java:63) at org.apache.zookeeper.server.quorum.QuorumPacket.deserialize(QuorumPacket.java:83) at org.apache.jute.BinaryInputArchive.readRecord(BinaryInputArchive.java:99) at org.apache.zookeeper.server.quorum.LearnerHandler.run(LearnerHandler.java:515) ERROR org.apache.zookeeper.server.quorum.LearnerHandler Unexpected exception causing shutdown while sock still open [LearnerHandler-/127.0.0.1:41758] java.io.IOException: Follower is ahead of the leader, leader summary: 1 (current epoch), 4294967297 (last zxid) at org.apache.zookeeper.server.quorum.Leader.waitForEpochAck(Leader.java:1209) at org.apache.zookeeper.server.quorum.LearnerHandler.run(LearnerHandler.java:421) INFO org.apache.curator.ConnectionState Connection string changed to: 127.0.0.1:33568,127.0.0.1:35059,127.0.0.1:39762 [main-EventThread] DEBUG org.apache.curator.ConnectionState reset [main-EventThread] ERROR org.apache.zookeeper.server.quorum.LearnerHandler Unexpected exception causing shutdown while sock still open [LearnerHandler-/127.0.0.1:41768] java.io.IOException: Follower is ahead of the leader, leader summary: 1 (current epoch), 4294967297 (last zxid) at org.apache.zookeeper.server.quorum.Leader.waitForEpochAck(Leader.java:1209) at org.apache.zookeeper.server.quorum.LearnerHandler.run(LearnerHandler.java:421) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #221: [CURATOR-411] Fix various testing issues
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/221 TestFailedDeleteManager seems to be ok now, but I'm still seeing fairly regular failure on TestReconfiguration.testNewMembers(). It's getting a ConnectionLossException at Line 313. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #221: [CURATOR-411] Fix various testing issues
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/221 The reconfiguration case seems to be better now. I'm still seeing failures on the TestFailedDeleteManager.testLostSession It seems that at line 77, it is sometimes getting an expired session instead of a lost connection. I wonder if this has something to do with the new way of determining LOST events when disconnected? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #221: [CURATOR-411] Fix various testing issues
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/221 I'm still seeing the following fail. The TestFailedDeleteManager case is intermittent, the reconfig one seems to be failing consistently. Failed tests: org.apache.curator.framework.imps.TestFailedDeleteManager.testWithNamespaceAndLostSessionAlt(org.apache.curator.framework.imps.TestFailedDeleteManager) Run 1: PASS Run 2: TestFailedDeleteManager.testWithNamespaceAndLostSessionAlt:203 » SessionExpired Run 3: PASS org.apache.curator.framework.imps.TestReconfiguration.testNewMembers(org.apache.curator.framework.imps.TestReconfiguration) Run 1: PASS Run 2: TestReconfiguration.testNewMembers:313 » ConnectionLoss KeeperErrorCode = Conn... Run 3: PASS Run 4: TestReconfiguration.testNewMembers:313 » CuratorConnectionLoss KeeperErrorCode... --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #218: [CURATOR-407] Added ttl suppport to Create transa...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/218#discussion_r114693246
--- Diff:
curator-x-async/src/main/java/org/apache/curator/x/async/details/AsyncTransactionOpImpl.java
---
@@ -87,10 +87,7 @@ public AsyncTransactionCreateBuilder create()
@Override
public AsyncPathAndBytesable withOptions(CreateMode
createMode, List aclList, boolean compressed)
{
-this.createMode = Objects.requireNonNull(createMode,
"createMode cannot be null");
-this.aclList = aclList;
-this.compressed = compressed;
-return this;
+return withOptions(createMode, aclList, compressed, ttl);
}
--- End diff --
Ok, 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #218: [CURATOR-407] Added ttl suppport to Create transa...
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/218#discussion_r114674883 --- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/transaction/TransactionCreateBuilder.java --- @@ -18,17 +18,17 @@ */ package org.apache.curator.framework.api.transaction; -import org.apache.curator.framework.api.ACLCreateModePathAndBytesable; -import org.apache.curator.framework.api.ACLPathAndBytesable; -import org.apache.curator.framework.api.Compressible; -import org.apache.curator.framework.api.CreateModable; -import org.apache.curator.framework.api.PathAndBytesable; - -public interface TransactionCreateBuilder extends -PathAndBytesable, -CreateModable>, -ACLPathAndBytesable, -ACLCreateModePathAndBytesable, -Compressible> +public interface TransactionCreateBuilder extends TransactionCreateBuilder2 --- End diff -- Yeah, it looks cool, I'll have to have a play when I get some time. Unfortunately, I'm not doing anything ZK related at work at the moment, so it will have to wait for now. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #218: [CURATOR-407] Added ttl suppport to Create transa...
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/218#discussion_r114673449 --- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/transaction/TransactionCreateBuilder.java --- @@ -18,17 +18,17 @@ */ package org.apache.curator.framework.api.transaction; -import org.apache.curator.framework.api.ACLCreateModePathAndBytesable; -import org.apache.curator.framework.api.ACLPathAndBytesable; -import org.apache.curator.framework.api.Compressible; -import org.apache.curator.framework.api.CreateModable; -import org.apache.curator.framework.api.PathAndBytesable; - -public interface TransactionCreateBuilder extends -PathAndBytesable, -CreateModable>, -ACLPathAndBytesable, -ACLCreateModePathAndBytesable, -Compressible> +public interface TransactionCreateBuilder extends TransactionCreateBuilder2 --- End diff -- Yep good point. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #218: [CURATOR-407] Added ttl suppport to Create transa...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/218#discussion_r114670214
--- Diff:
curator-x-async/src/main/java/org/apache/curator/x/async/details/AsyncTransactionOpImpl.java
---
@@ -77,6 +78,13 @@ public AsyncTransactionCreateBuilder create()
}
@Override
+public AsyncPathAndBytesable withTtl(long ttl)
+{
+this.ttl = ttl;
+return this;
+}
+
+@Override
public AsyncPathAndBytesable withOptions(CreateMode
createMode, List aclList, boolean compressed)
{
this.createMode = Objects.requireNonNull(createMode,
"createMode cannot be null");
--- End diff --
Can't this just call the newly overloaded withOptions and pass in a TTL of
-1?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #218: [CURATOR-407] Added ttl suppport to Create transa...
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/218#discussion_r114670616 --- Diff: curator-framework/src/main/java/org/apache/curator/framework/api/transaction/TransactionCreateBuilder.java --- @@ -18,17 +18,17 @@ */ package org.apache.curator.framework.api.transaction; -import org.apache.curator.framework.api.ACLCreateModePathAndBytesable; -import org.apache.curator.framework.api.ACLPathAndBytesable; -import org.apache.curator.framework.api.Compressible; -import org.apache.curator.framework.api.CreateModable; -import org.apache.curator.framework.api.PathAndBytesable; - -public interface TransactionCreateBuilder extends -PathAndBytesable, -CreateModable>, -ACLPathAndBytesable, -ACLCreateModePathAndBytesable, -Compressible> +public interface TransactionCreateBuilder extends TransactionCreateBuilder2 --- End diff -- I'm sure there's a reason for this approach, but can you enlighten me as to why you don't just add the withTtl() method to the TransactionCreateBuilder and not introduce the TransactionCreateBuilder2? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #171: [CURATOR-351] Support for TTL Nodes
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/171#discussion_r111854064
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java
---
@@ -0,0 +1,58 @@
+package org.apache.curator.framework.recipes.nodes;
+
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.BaseClassForTests;
+import org.apache.curator.test.Timing;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+import java.util.concurrent.TimeUnit;
+
+public class TestPersistentTtlNode extends BaseClassForTests
+{
+private final Timing timing = new Timing();
+
+@BeforeMethod
+@Override
+public void setup() throws Exception
+{
+System.setProperty("znode.container.checkIntervalMs", "1");
+super.setup();
+}
+
+@AfterMethod
+@Override
+public void teardown() throws Exception
+{
+System.clearProperty("znode.container.checkIntervalMs");
+super.teardown();
+}
+
+@Test
+public void testBasic() throws Exception
+{
+try (CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1)))
+{
+client.start();
+
+try (PersistentTtlNode node = new PersistentTtlNode(client,
"/test", 10, new byte[0]))
+{
+node.start();
+node.waitForInitialCreate(timing.session(),
TimeUnit.MILLISECONDS);
--- End diff --
This should probably assert that the return is true?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #171: [CURATOR-351] Support for TTL Nodes
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/171#discussion_r111854284
--- Diff:
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java
---
@@ -44,25 +45,49 @@ public void testBasic() throws Exception
{
client.start();
-try (PersistentTtlNode node = new PersistentTtlNode(client,
"/test", 10, new byte[0]))
+try (PersistentTtlNode node = new PersistentTtlNode(client,
"/test", 100, new byte[0]))
{
node.start();
node.waitForInitialCreate(timing.session(),
TimeUnit.MILLISECONDS);
--- End diff --
This should probably assert that the return is true?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #171: [CURATOR-351] Support for TTL Nodes
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/171#discussion_r111849959
--- Diff:
curator-framework/src/test/java/org/apache/curator/framework/imps/TestTtlNodes.java
---
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.imps;
+
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.BaseClassForTests;
+import org.apache.curator.test.Timing;
+import org.apache.zookeeper.CreateMode;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+import java.util.concurrent.CountDownLatch;
+
+public class TestTtlNodes extends BaseClassForTests
+{
+@BeforeMethod
+@Override
+public void setup() throws Exception
+{
+System.setProperty("znode.container.checkIntervalMs", "1");
+super.setup();
+}
+
+@AfterMethod
+@Override
+public void teardown() throws Exception
+{
+super.teardown();
+System.clearProperty("znode.container.checkIntervalMs");
+}
+
+@Test
+public void testBasic() throws Exception
+{
+try ( CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1)) )
+{
+client.start();
+
+
client.create().withTtl(10).creatingParentsIfNeeded().withMode(CreateMode.PERSISTENT_WITH_TTL).forPath("/a/b/c");
+Thread.sleep(20);
--- End diff --
You're probably right, my concern is that I don't know how quick ZK
responds to these TTL expiry events. You've only got a 10ms window here for ZK
to remove the node. Maybe that's enough.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #213: [CURATOR-395] Potential null dereference in PersistentNo...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/213 Ah, I see now. It's the Preconditions.checkState call that causes the issue. I thought that we could just hold a reference to the builder and then call forPath() on it when required. But that checkState call will fail if the client is not started. So, the PR looks OK to me. Merge away. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #213: [CURATOR-395] Potential null dereference in PersistentNo...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/213 I think that the fix is fine, but why is the createMethod a lazily instantiated reference? Couldn't it just be a normal reference that gets created in the constructor? We seem to have everything we need there to create it. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #208: [CURATOR-394] UnrecognizedPropertyException: "enabled" i...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/208 I can't think of a better option other than deprecating the 1 arg constructor, and forcing clients to explicitly define their behaviour going forward. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #208: [CURATOR-394] UnrecognizedPropertyException: "enabled" i...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/208 I think that this is OK. I'm just wondering what the best way forward is after this release? Ideally we don't want to have to have users opt in to pick up CURATOR-275 do we? I've never used the service discovery stuff, so I'm not sure if that's an issue. Should the next release after the one that includes this have the serialization of enabled set to true? Then we would have an upgrade path from pre 2.12.x to 2.12.x+1 (with this fix) to 2.12.x + 2? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #208: [CURATOR-394] UnrecognizedPropertyException: "ena...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/208#discussion_r108049761
--- Diff:
curator-x-discovery/src/test/java/org/apache/curator/x/discovery/details/NewServiceInstance.java
---
@@ -0,0 +1,148 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.x.discovery.details;
+
+import com.google.common.base.Preconditions;
+import org.apache.curator.x.discovery.ServiceType;
+import org.apache.curator.x.discovery.UriSpec;
+import org.codehaus.jackson.annotate.JsonTypeInfo;
+import org.codehaus.jackson.annotate.JsonTypeInfo.Id;
+import java.net.URI;
+import java.util.Date;
+
+/**
+ * POJO that represents a service instance
+ */
+class NewServiceInstance
+{
+private final String name;
+private final String id;
+private final String address;
+private final Integer port;
+private final Integer sslPort;
+private final T payload;
+private final long registrationTimeUTC;
+private final ServiceType serviceType;
+private final UriSpec uriSpec;
+private final boolean enabled;
+private final String new1;
+private final Long new2;
+private final Date new3;
+private final URI new4;
+
--- End diff --
What are these new1-new4 variables? They seem to be a new addition as part
of this fix?
Also, Is there a reason that this class can't be a subclass of the
OldServiceInstance?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #208: [CURATOR-394] UnrecognizedPropertyException: "ena...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/208#discussion_r108049783
--- Diff:
curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/JsonInstanceSerializer.java
---
@@ -16,30 +16,57 @@
* specific language governing permissions and limitations
* under the License.
*/
+
package org.apache.curator.x.discovery.details;
+import com.google.common.annotations.VisibleForTesting;
import org.apache.curator.x.discovery.ServiceInstance;
+import org.codehaus.jackson.map.DeserializationConfig;
import org.codehaus.jackson.map.ObjectMapper;
import org.codehaus.jackson.type.JavaType;
-import java.io.ByteArrayOutputStream;
/**
* A serializer that uses Jackson to serialize/deserialize as JSON.
IMPORTANT: The instance
* payload must support Jackson
*/
public class JsonInstanceSerializer implements InstanceSerializer
{
-private final ObjectMapper mapper;
-private final Class payloadClass;
-private final JavaType type;
+private final ObjectMapper mapper;
+private final Class payloadClass;
+private final boolean compatibleSerializationMode;
+private final JavaType type;
/**
* @param payloadClass used to validate payloads when deserializing
*/
public JsonInstanceSerializer(Class payloadClass)
{
+this(payloadClass, false, false);
--- End diff --
Doesn't this approach imply that moving to Curator 2.12.x requires you to
change the code base to not break backwards compatibility? Shouldn't the
default be to be in compatibility mode? Then you can just upgrade to 2.12.x and
everything will still work, but you need to change the code base to pick up the
fix for CURATOR-275?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #204: [CURATOR-378] Update mvn parent pom, plugin and o...
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/204#discussion_r105804310 --- Diff: pom.xml --- @@ -62,22 +62,22 @@ 3.4.8 - 2.7 -2.3.7 -2.10.3 - 1.6 + 2.9 +3.2.0 +2.10.4 + 1.7 1.9.0 -3.18.1-GA +3.20.0-GA 2.2 1.9.13 1.18.1 1.1.1 6.1.26 1.0.2 2.3.0.GA -16.0.1 +19.0 --- End diff -- I don't really have a strong preference either way, I don't use guava a great deal outside Curator, so I'm happy to defer to those who use it more. @Randgalt? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #204: [CURATOR-378] Update mvn parent pom, plugin and o...
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/204#discussion_r105800775 --- Diff: pom.xml --- @@ -62,22 +62,22 @@ 3.4.8 - 2.7 -2.3.7 -2.10.3 - 1.6 + 2.9 +3.2.0 +2.10.4 + 1.7 1.9.0 -3.18.1-GA +3.20.0-GA 2.2 1.9.13 1.18.1 1.1.1 6.1.26 1.0.2 2.3.0.GA -16.0.1 +19.0 --- End diff -- Is there a reason this is 19.0 instead of 20.0? I see we can't go to 21.0 because it's got a Java 1.8 dependency, but 20 should be ok? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #103: [CURATOR-259] Added Locker which uses Java 7's try-with-...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/103 Curator 2.x only supports Java 1.6 so can't support the auto close features in Java 1.7 Curator 3.x supports Java 1.7. The Locker class is present in this version. cheers On Wed, Mar 8, 2017 at 4:20 AM, Paul Vorbach wrote: > This class is no longer available. Is this correct? > > It is still mentioned on http://curator.apache.org/utilities.html, which > is why I found this PR. > > â > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <https://github.com/apache/curator/pull/103#issuecomment-284792965>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AHZ0sd0PMT71pLepftwyaVl-Gghrgbjeks5rjZH1gaJpZM4F4pym> > . > --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/197#discussion_r103351183
--- Diff:
curator-client/src/main/java/org/apache/curator/ConnectionState.java ---
@@ -283,12 +300,19 @@ private boolean checkState(Event.KeeperState state,
boolean wasConnected)
new EventTrace(state.toString(), tracer.get(),
getSessionId()).commit();
}
-if ( checkNewConnectionString &&
zooKeeper.hasNewConnectionString() )
+return isConnected;
+}
+
+private void handleState(Event.KeeperState state)
+{
+if (state == Event.KeeperState.Expired)
+{
+handleExpiredSession();
--- End diff --
Isn't that true of the existing implementation though? If the state is
expired then the checkNewConnectionString flag gets set to false so the
handleNewConnectionString() method won't get called.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/197#discussion_r103338590
--- Diff:
curator-client/src/main/java/org/apache/curator/ConnectionState.java ---
@@ -283,12 +300,19 @@ private boolean checkState(Event.KeeperState state,
boolean wasConnected)
new EventTrace(state.toString(), tracer.get(),
getSessionId()).commit();
}
-if ( checkNewConnectionString &&
zooKeeper.hasNewConnectionString() )
+return isConnected;
+}
+
+private void handleState(Event.KeeperState state)
+{
+if (state == Event.KeeperState.Expired)
+{
+handleExpiredSession();
--- End diff --
It gets handled in the handleState() method, which is called from process()
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/197 Any thoughts on this @Randgalt , I'd like to merge before doing a build. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/197 I've run through some more tests and it all looks good. My only concern is that we're introducing the Mockito just for this test case. Could you move the test case into the curator-client sub package under org.apache.curator. Then no reflection would be required to set the debug flag and we could remove the mockito dependency? Do you have an opinion on introducing the Mockito dependency @Randgalt? In regards to the Curator 2.x and 3.x, don't worry about a separate PR, it should be ok to just merge from the Curator 2.x branch into 3.x. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/197 Thanks for the update, looks OK now I think. I'll give it a bit more of a test early next week and then merge. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/197#discussion_r98551865 --- Diff: curator-client/src/main/java/org/apache/curator/ConnectionState.java --- @@ -160,13 +162,33 @@ public void process(WatchedEvent event) } } +// only wait during tests +assert waitOnExpiredEvent(event.getState()); --- End diff -- I don't think that it's reasonable to assume that assertions will only be turned on during testing. If you look at something like LeaderSelector, it has specific code in there to support unit testing (the debugLeadershipLatch variable). If you can cause the problem to occur without this code though, then it should be removed. I had a quick play with it and I couldn't seem to reproduce it without this code though. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/197#discussion_r98367381
--- Diff:
curator-client/src/main/java/org/apache/curator/ConnectionState.java ---
@@ -160,13 +162,33 @@ public void process(WatchedEvent event)
}
}
+// only wait during tests
+assert waitOnExpiredEvent(event.getState());
+
for ( Watcher parentWatcher : parentWatchers )
{
-
OperationTrace trace = new
OperationTrace("connection-state-parent-process", tracer.get(), getSessionId());
parentWatcher.process(event);
trace.commit();
}
+
+if (eventTypeNone) handleState(event.getState());
+}
+
+// only for testing
+private boolean waitOnExpiredEvent(Event.KeeperState currentState)
--- End diff --
What is the purpose of the boolean return? The method only ever returns
true? Is it purely so you can call it from the assert?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #197: [CURATOR-367] Delay reconnect on session expired
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/197#discussion_r98367367 --- Diff: curator-client/src/main/java/org/apache/curator/ConnectionState.java --- @@ -160,13 +162,33 @@ public void process(WatchedEvent event) } } +// only wait during tests +assert waitOnExpiredEvent(event.getState()); --- End diff -- How is this meant to work? Are you assuming that assertions will only be enabled in testing? If you need some logic to only be executed during testing shouldn't there be some sort of internal flag indicating 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #189: [CURATOR-99] Java 8 DSL for Curator
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/189 I don't have any real experience with the Java 8 asynch framework, but what you've implemented looks good to me. I'll have a look at the doco next week when I get some time (on leave at the moment). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #186: [CURATOR-375] - fix thread interruption being reported t...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/186 Is this fix specifically for the PersistentNode class? It seems like it will potentially break a lot of stuff. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #186: [CURATOR-375] - fix thread interruption being rep...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/186#discussion_r95088443
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -485,7 +485,6 @@ private String protectedPathInForeground(String
adjustedPath, byte[] data) throw
}
catch ( Exception e)
{
-ThreadUtils.checkInterrupted(e);
--- End diff --
Won't the removal of this call mean that the thread will not be marked as
interrupted unless the caller catches the exception and resets the value
themselves? This code is called from many recipes and can also be called
explicitly by users Curator code. I would think that there's great potential to
break code here, unless I'm missing something.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator issue #178: [CURATOR-365] backgroundCreateParentsThenNode() ignores ...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/178 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #175: [CURATOR-360] Allow TestingCluster to listen on other ne...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/175 Looks good to me. I assume there's no easy way of providing unit tests for the new functionality? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #173: CURATOR-358 - Fixed race condition with getLeader...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/173#discussion_r89223241
--- Diff:
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderSelector.java
---
@@ -341,11 +342,41 @@ public Participant getLeader() throws Exception
static Participant getLeader(CuratorFramework client,
Collection participantNodes) throws Exception
{
+Participant result = null;
+
if ( participantNodes.size() > 0 )
{
-return participantForPath(client,
participantNodes.iterator().next(), true);
+Iterator iter = participantNodes.iterator();
+while ( iter.hasNext() )
+{
+
+try
+{
+result = participantForPath(client, iter.next(), true);
--- End diff --
Yep, fair call, it's probably more Java like to just return. I've probably
been writing too much C code lately!
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #173: CURATOR-358 - Fixed race condition with getLeader...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/173#discussion_r89212147
--- Diff:
curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderSelector.java
---
@@ -341,11 +342,41 @@ public Participant getLeader() throws Exception
static Participant getLeader(CuratorFramework client,
Collection participantNodes) throws Exception
{
+Participant result = null;
+
if ( participantNodes.size() > 0 )
{
-return participantForPath(client,
participantNodes.iterator().next(), true);
+Iterator iter = participantNodes.iterator();
+while ( iter.hasNext() )
+{
+
+try
+{
+result = participantForPath(client, iter.next(), true);
--- End diff --
Any other comments?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #173: CURATOR-358 - Fixed race condition with getLeader...
GitHub user cammckenzie opened a pull request: https://github.com/apache/curator/pull/173 CURATOR-358 - Fixed race condition with getLeader() -If leadership changes between the getParticipantNodes() call and the getLeader() internal call the NoNodeException is now handled and the next child in the list is evaluated. Another option would be to just return the default empty Participant object and not iterate over the whole list of participants. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/curator CURATOR-358 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/curator/pull/173.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 #173 commit 3478aca7ed6852484b5574a6082f4bb75c04a1e0 Author: Cam McKenzie Date: 2016-11-20T23:38:15Z CURATOR-358 - Fixed race condition with getLeader() -If leadership changes between the getParticipantNodes() call and the getLeader() internal call the NoNodeException is now handled and the next child in the list is evaluated. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator pull request #171: [CURATOR-351] Support for TTL Nodes - NOT READY F...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/171#discussion_r86286940
--- Diff:
curator-framework/src/test/java/org/apache/curator/framework/imps/TestTtlNodes.java
---
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.framework.imps;
+
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.framework.api.BackgroundCallback;
+import org.apache.curator.framework.api.CuratorEvent;
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.BaseClassForTests;
+import org.apache.curator.test.Timing;
+import org.apache.zookeeper.CreateMode;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+import java.util.concurrent.CountDownLatch;
+
+public class TestTtlNodes extends BaseClassForTests
+{
+@BeforeMethod
+@Override
+public void setup() throws Exception
+{
+System.setProperty("znode.container.checkIntervalMs", "1");
+super.setup();
+}
+
+@AfterMethod
+@Override
+public void teardown() throws Exception
+{
+super.teardown();
+System.clearProperty("znode.container.checkIntervalMs");
+}
+
+@Test
+public void testBasic() throws Exception
+{
+try ( CuratorFramework client =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(1)) )
+{
+client.start();
+
+
client.create().withTtl(10).creatingParentsIfNeeded().withMode(CreateMode.PERSISTENT_WITH_TTL).forPath("/a/b/c");
+Thread.sleep(20);
--- End diff --
I assume that client should get a 'removed' event when the node times out
and gets removed by ZK? If so, wouldn't it be less error prone to wait on a
latch and checking that the node was deleted within 10 +- some fudge factor
rather than just sleeping?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] curator pull request #171: [CURATOR-351] Support for TTL Nodes - NOT READY F...
Github user cammckenzie commented on a diff in the pull request: https://github.com/apache/curator/pull/171#discussion_r86286992 --- Diff: curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java --- @@ -83,6 +85,13 @@ public CreateBuilderMain orSetData() return this; } +@Override +public CreateBuilderMain withTtl(long ttl) --- End diff -- I prefer withTTL, but that's just personal preference. I'm not sure what the standard for capitalisation of acronyms is. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #170: CURATOR-356 Allow SASL configuration for TestingServer
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/170 Looks good to me, thanks for the PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/165 Thanks @lvfangmin, unfortunately I'm somewhat geographically challenged living on the other side of the world. Sounds like fun though. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #168: CURATOR-354: native memory leak in GzipCompressionProvid...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/168 Thanks for the PR! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #167: [CURATOR-352] SchemaViolation errors do not contain enou...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/167 Looks ok 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #166: [CURATOR-345] clientAddr might be null in which case add...
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/166 Looks ok 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] curator issue #165: [CURATOR-349] Expose extra metrics in TracerDriver
Github user cammckenzie commented on the issue: https://github.com/apache/curator/pull/165 I've had a bit more of a look and I think that the changes are OK. As you mentioned, the changes are only to internal classes. There is a loss of granularity (The TimeTrace was using nanos while your new implementation is using millis), but I doubt that this is an issue in practice. So, I'm happy with the changes. @Randgalt is away at the moment, but I think that he should review it when he returns. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
