-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CURATOR-558 - part 1 of ZK 3.6 updates #344
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks OK, other than the 1 comment about the unit test assertion ordering changing.
assertEvent(TreeCacheEvent.Type.INITIALIZED, null, null, true); | ||
assertEvent(TreeCacheEvent.Type.NODE_REMOVED, "/test/me", "data".getBytes(), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change of order of assertions deliberate (it was previously NODE_REMOVED, then INITIALIZED, it is now INITIALIZED then NODE_REMOVED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one in ZooKeeperTestable works slightly differently - that's probably why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rightyo, I thought it was calling the same code under the covers. Anyway, I'm OK with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. The different is that zookeeper's testable has these additional lines
this.clientCnxn.eventThread.queueEventOfDeath();
this.clientCnxn.state = States.CLOSED;
this.clientCnxn.sendThread.getClientCnxnSocket().onClosing();
where both of them calls
this.clientCnxn.eventThread.queueEvent(new WatchedEvent(EventType.None, KeeperState.Expired, (String)null));
I'm not very sure how it changes the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote the ZK testable version but, tbh, I can't remember the specifics.
e943f7a
to
45a6665
Compare
5574e4d
to
48434af
Compare
48434af
to
e6424b4
Compare
@cammckenzie @shayshim @tisonkun I'm going to merge this unless there is objection. Speak now if you have any concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening the pull request @Randgalt !
LGTM. I left one minor inline comment and be curious that we still keep curator-test-zk34
module. It seems we can remove it in this pass, doesn't it?
* Upgraded version of ListenerContainer that | ||
* doesn't leak Guava's internals and also supports mapping/wrapping of listeners |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Upgraded version of ListenerContainer that | |
* doesn't leak Guava's internals and also supports mapping/wrapping of listeners | |
* 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.
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good! I've thought of these changes some days ago.
e6424b4
to
c4e9432
Compare
Pt1 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
c4e9432
to
3f0f951
Compare
Pt1 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 Co-authored-by: randgalt <[email protected]>
Pt 1 of change