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/03 22:15:24 UTC
[GitHub] [curator] Randgalt opened a new pull request #345: [CURATOR-551]
Fix Handle Holder new connection string
Randgalt opened a new pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345
Commit 26364c6186fc7c09a9462557b1ca791e9aa70006 (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.
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r374374654
##########
File path: 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);
Review comment:
Also here - this is the meat of the change
----------------------------------------------------------------
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] shayshim commented on a change in pull request #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r378038955
##########
File path: 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();
Review comment:
> 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
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377460199
##########
File path: 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();
Review comment:
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.
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r374373331
##########
File path: 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();
Review comment:
Note: this is the major change
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377970470
##########
File path: 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();
Review comment:
> 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.
----------------------------------------------------------------
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] shayshim commented on a change in pull request #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r378533404
##########
File path: 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();
Review comment:
> `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()` 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.
----------------------------------------------------------------
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] shayshim commented on a change in pull request #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377876821
##########
File path: 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();
Review comment:
I see, thanks @cammckenzie
----------------------------------------------------------------
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] shayshim commented on a change in pull request #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377292749
##########
File path: 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();
Review comment:
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`.
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377896735
##########
File path: 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();
Review comment:
I'm OK with that. @shayshim ? Maybe you can both approve this PR when you're OK
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377607990
##########
File path: 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();
Review comment:
> whole flow of how things interact is very confusing
Too true. What do we do? Should we attempt a more aggressive change?
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r374373265
##########
File path: curator-client/src/main/java/org/apache/curator/HandleHolder.java
##########
@@ -34,15 +34,6 @@
private volatile Helper helper;
- private interface Helper
Review comment:
Moved to a top level class so imp can be shared and it's easier to read.
----------------------------------------------------------------
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] shayshim commented on a change in pull request #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r378533404
##########
File path: 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();
Review comment:
> `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.
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r374373071
##########
File path: 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;
Review comment:
Note: renamed for clarity
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377438443
##########
File path: 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();
Review comment:
Thanks @shayshim I'll look into this possiblity
----------------------------------------------------------------
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 #345: [CURATOR-551] Fix
Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
Randgalt merged pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345
----------------------------------------------------------------
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 #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r377894735
##########
File path: 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();
Review comment:
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.
----------------------------------------------------------------
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] shayshim commented on a change in pull request #345:
[CURATOR-551] Fix Handle Holder new connection string
Posted by GitBox <gi...@apache.org>.
shayshim commented on a change in pull request #345: [CURATOR-551] Fix Handle Holder new connection string
URL: https://github.com/apache/curator/pull/345#discussion_r378533404
##########
File path: 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();
Review comment:
> `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()` 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.
----------------------------------------------------------------
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