[CURATOR-551] Fix Handle Holder new connection string #345

Randgalt · 2020-02-03T22:15:23Z

Commit 26364c6 (Sat Sep 26 13:13:02 2015) changed HandleHolder.getNewConnectionString() was changed to return the new connection string instead of just a boolean. I believe the value returned should have been ensembleProvider.getConnectionString() not helper.getConnectionString(). TBH I no longer remember the genesis of this change but I can't make the current implementation make sense.

Additionally, a change was made to optionally call zooKeeper.updateServerList(). When this path is taken the handle holder's connection needs to be updated as well or we'll get an infinite loop of changes. The path that runs when ensembleProvider.updateServerListEnabled() is false ends up setting handle holder's connection to ensembleProvider.getConnectionString().

I'm loathe to make such low level changes in code that's existed for a long time. But, my investigation shows that this is how it should be. Hopefully, users can do testing.

Commit 26364c6 (Sat Sep 26 13:13:02 2015) changed HandleHolder.getNewConnectionString() was changed to return the new connection string instead of just a boolean. I believe the value returned should have been ensembleProvider.getConnectionString() not helper.getConnectionString(). TBH I no longer remember the genesis of this change but I can't make the current implementation make sense. Additionally, a change was made to optionally call zooKeeper.updateServerList(). When this path is taken the handle holder's connection needs to be updated as well or we'll get an infinite loop of changes. The path that runs when ensembleProvider.updateServerListEnabled() is false ends up setting handle holder's connection to ensembleProvider.getConnectionString(). I'm loathe to make such low level changes in code that's existed for a long time. But, my investigation shows that this is how it should be. Hopefully, users can do testing.

Randgalt · 2020-02-03T22:17:13Z

curator-client/src/main/java/org/apache/curator/ConnectionState.java

@@ -48,7 +48,7 @@
    private static final int MAX_BACKGROUND_EXCEPTIONS = 10;
    private static final boolean LOG_EVENTS = Boolean.getBoolean(DebugUtils.PROPERTY_LOG_EVENTS);
    private static final Logger log = LoggerFactory.getLogger(ConnectionState.class);
-    private final HandleHolder zooKeeper;
+    private final HandleHolder handleHolder;


Note: renamed for clarity

Randgalt · 2020-02-03T22:17:44Z

curator-client/src/main/java/org/apache/curator/HandleHolder.java

@@ -34,15 +34,6 @@

    private volatile Helper helper;

-    private interface Helper


Moved to a top level class so imp can be shared and it's easier to read.

Randgalt · 2020-02-03T22:17:55Z

curator-client/src/main/java/org/apache/curator/HandleHolder.java

@@ -70,7 +61,16 @@ String  getConnectionString()
    String getNewConnectionString()
    {
        String helperConnectionString = (helper != null) ? helper.getConnectionString() : null;
-        return ((helperConnectionString != null) && !ensembleProvider.getConnectionString().equals(helperConnectionString)) ? helperConnectionString : null;
+        String ensembleProviderConnectionString = ensembleProvider.getConnectionString();


Note: this is the major change

I have a question. helper is null until closeAndReset() is called, and new connection string can be sensed only if helper is not null. Isn't it possible to have new connection string (from the EnsembleTracker) before closeAndReset() was ever called? If it is possible, then it seems like the new connection string won't be handled, since helper would be null.

Thanks @shayshim I'll look into this possiblity

The whole flow of how things interact is very confusing, but I don't think that this is an issue. closeAndReset() gets called from ConnectionState.reset() which gets called from the ConnectionState.start() method. I'm not 100% sure there's not a race condition here, but from what I can see, it seems improbable.

whole flow of how things interact is very confusing

Too true. What do we do? Should we attempt a more aggressive change?

Maybe we should just merge this change and look at cleaning up this stuff in a future PR? I think it would be a pretty significant change.

I'm OK with that. @shayshim ? Maybe you can both approve this PR when you're OK

I have a question. helper is null until closeAndReset() is called, and new connection string can be sensed only if helper is not null. Isn't it possible to have new connection string (from the EnsembleTracker) before closeAndReset() was ever called? If it is possible, then it seems like the new connection string won't be handled, since helper would be null.

closeAndReset() will cause ensembleProvider.getConnectionString() to get called anyway. Does that answer your question? I think the intent of the above code is to force-close the connection when there's a new connection string.

I'm OK with that. @shayshim ? Maybe you can both approve this PR when you're OK

I agree - better to continue refactor in different PR, dedicated for that

closeAndReset() will cause ensembleProvider.getConnectionString() to get called anyway. Does that answer your question? I think the intent of the above code is to force-close the connection when there's a new connection string.

In short I wanted to make sure that closeAndReset() (create helper) is called before any new connection string is set .
As @cammckenzie said, it is called from ConnectionState.start(), which is called (eventually) from CuratorFrameworkImpl.start(), from line 338 using client.start() - before ensembleTracker.start() in line 353, which allows to set new connection strings.
So any call to getNewConnectionString(), with actual new connection string, will meet non null helper.
In addition to that, helper.getZooKeeper() is also called eventually from 338, so data.connectionString is set as needed, before getNewConnectionString() can be called.
So I think we have no issue here.

Randgalt · 2020-02-03T22:20:57Z

curator-client/src/main/java/org/apache/curator/ConnectionState.java

@@ -356,6 +356,7 @@ private void handleNewConnectionString(String newConnectionString)
                if ( ensembleProvider.updateServerListEnabled() )
                {
                    zooKeeper.updateServerList(newConnectionString);
+                    handleHolder.resetConnectionString(newConnectionString);


Also here - this is the meat of the change

shayshim

@Randgalt looks fine to me

cammckenzie

Looks OK to me.

shayshim

LGTM

Commit 26364c6 (Sat Sep 26 13:13:02 2015) changed HandleHolder.getNewConnectionString() was changed to return the new connection string instead of just a boolean. I believe the value returned should have been ensembleProvider.getConnectionString() not helper.getConnectionString(). TBH I no longer remember the genesis of this change but I can't make the current implementation make sense. Additionally, a change was made to optionally call zooKeeper.updateServerList(). When this path is taken the handle holder's connection needs to be updated as well or we'll get an infinite loop of changes. The path that runs when ensembleProvider.updateServerListEnabled() is false ends up setting handle holder's connection to ensembleProvider.getConnectionString(). I'm loathe to make such low level changes in code that's existed for a long time. But, my investigation shows that this is how it should be. Hopefully, users can do testing.

Randgalt commented Feb 3, 2020

View reviewed changes

shayshim reviewed Feb 11, 2020

View reviewed changes

cammckenzie approved these changes Feb 11, 2020

View reviewed changes

Randgalt requested a review from shayshim February 11, 2020 23:54

shayshim approved these changes Feb 12, 2020

View reviewed changes

Randgalt merged commit 75226fb into master Feb 12, 2020

tisonkun deleted the CURATOR-551-fix-handle-holder-new-connection-string branch March 13, 2023 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CURATOR-551] Fix Handle Holder new connection string #345

[CURATOR-551] Fix Handle Holder new connection string #345

Randgalt commented Feb 3, 2020

Randgalt Feb 3, 2020

Randgalt Feb 3, 2020

Randgalt Feb 3, 2020

shayshim Feb 10, 2020

Randgalt Feb 11, 2020

cammckenzie Feb 11, 2020

Randgalt Feb 11, 2020

cammckenzie Feb 11, 2020

Randgalt Feb 11, 2020

Randgalt Feb 11, 2020

shayshim Feb 12, 2020

shayshim Feb 12, 2020 •

edited

Loading

Randgalt Feb 3, 2020

shayshim left a comment

cammckenzie left a comment

shayshim left a comment

		@@ -34,15 +34,6 @@

		private volatile Helper helper;

		private interface Helper

[CURATOR-551] Fix Handle Holder new connection string #345

[CURATOR-551] Fix Handle Holder new connection string #345

Conversation

Randgalt commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shayshim Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shayshim left a comment

Choose a reason for hiding this comment

cammckenzie left a comment

Choose a reason for hiding this comment

shayshim left a comment

Choose a reason for hiding this comment

shayshim Feb 12, 2020 •

edited

Loading