You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2020/02/02 16:36:43 UTC
[GitHub] [curator] Randgalt opened a new pull request #344: CURATOR-558 -
part 1 of ZK 3.6 updates
Randgalt opened a new pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344
Pt 1 of change
* Remove the ZK 3.4 compatibility module and code
* Remove the deprecated ListenerContainer that leaks Guava classes into our APIs
* Remove Exhibitor support
* Various minor changes/cleanups
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r374460709
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
The one in ZooKeeperTestable works slightly differently - that's probably why.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] TisonKun commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r392620893
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/listen/MappingListenerManager.java
##########
@@ -28,7 +28,7 @@
import java.util.function.Function;
/**
- * Upgraded version of {@link org.apache.curator.framework.listen.ListenerContainer} that
+ * Upgraded version of ListenerContainer that
* doesn't leak Guava's internals and also supports mapping/wrapping of listeners
Review comment:
```suggestion
* Subclass of {@link ListenerManager} that supports mapping/wrapping of listeners.
```
Since `ListenerContainer` removed, I don't think we have to mention this history in the nightly document.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r374446608
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
@cammckenzie I moved all kill session calls to use `zooKeeper.getTestable().injectSessionExpiration()`. When I did that, this particular test's message ordering changed. I didn't think it was too significant. Thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt merged pull request #344: CURATOR-558 - part 1
of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
Randgalt merged pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on issue #344: CURATOR-558 - part 1 of
ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
Randgalt commented on issue #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#issuecomment-599122337
@cammckenzie @shayshim @TisonKun I'm going to merge this unless there is objection. Speak now if you have any concerns.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] cammckenzie commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r374459055
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
Probably not significant, just noticed that the ordering had changed. Interesting that the session injection call has changed the ordering though, given that under the covers it is using the same mechanism.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] cammckenzie commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r374409491
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
Is this change of order of assertions deliberate (it was previously NODE_REMOVED, then INITIALIZED, it is now INITIALIZED then NODE_REMOVED
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] Randgalt commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r392623465
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
I wrote the ZK testable version but, tbh, I can't remember the specifics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] cammckenzie commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r374460952
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
Rightyo, I thought it was calling the same code under the covers. Anyway, I'm OK with it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] TisonKun commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r392620988
##########
File path: curator-recipes/src/main/java/org/apache/curator/framework/recipes/cache/PathChildrenCache.java
##########
@@ -74,7 +74,7 @@
private final CloseableExecutorService executorService;
private final boolean cacheData;
private final boolean dataIsCompressed;
- private final ListenerContainer<PathChildrenCacheListener> listeners = new ListenerContainer<PathChildrenCacheListener>();
+ private final StandardListenerManager<PathChildrenCacheListener> listeners = StandardListenerManager.standard();
Review comment:
Good! I've thought of these changes some days ago.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [curator] TisonKun commented on a change in pull request #344:
CURATOR-558 - part 1 of ZK 3.6 updates
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #344: CURATOR-558 - part 1 of ZK 3.6 updates
URL: https://github.com/apache/curator/pull/344#discussion_r392621920
##########
File path: curator-recipes/src/test/java/org/apache/curator/framework/recipes/cache/TestTreeCache.java
##########
@@ -423,9 +423,9 @@ public void testKilledSession() throws Exception
client.create().withMode(CreateMode.EPHEMERAL).forPath("/test/me", "data".getBytes());
assertEvent(TreeCacheEvent.Type.NODE_ADDED, "/test/me");
- Compatibility.injectSessionExpiration(client.getZookeeperClient().getZooKeeper());
- assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
+ client.getZookeeperClient().getZooKeeper().getTestable().injectSessionExpiration();
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true);
+ assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true);
Review comment:
Interesting. The different is that zookeeper's testable has these additional lines
```java
this.clientCnxn.eventThread.queueEventOfDeath();
this.clientCnxn.state = States.CLOSED;
this.clientCnxn.sendThread.getClientCnxnSocket().onClosing();
```
where both of them calls
```java
this.clientCnxn.eventThread.queueEvent(new WatchedEvent(EventType.None, KeeperState.Expired, (String)null));
```
I'm not very sure how it changes the behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services