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 2021/11/14 13:03:23 UTC
[GitHub] [curator] Randgalt opened a new pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Randgalt opened a new pull request #401:
URL: https://github.com/apache/curator/pull/401
`ChildrenCache` (used by Queues) didn't have a `ConnectionStateListener`. Thus, if a long network partition occurred the ZK instance would be recreated losing any set watcher and the ChildrenCache would fail to continue watching changes. Adding a ConnectionStateListener fixes this.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli commented on a change in pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #401:
URL: https://github.com/apache/curator/pull/401#discussion_r749267842
##########
File path: curator-recipes/src/main/java/org/apache/curator/framework/recipes/queue/ChildrenCache.java
##########
@@ -66,6 +68,19 @@ public void processResult(CuratorFramework client, CuratorEvent event) throws Ex
}
};
+ private final ConnectionStateListener connectionStateListener = (__, newState) -> {
+ if ((newState == ConnectionState.CONNECTED) || (newState == ConnectionState.RECONNECTED)) {
+ try
+ {
+ sync();
+ }
+ catch ( Exception e )
+ {
+ throw new RuntimeException(e);
Review comment:
will this error be logged somewhere ?
as we cannot perform any recovery
will it make more sense to simply log the exception and do not rethrow 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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] eolivelli commented on pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #401:
URL: https://github.com/apache/curator/pull/401#issuecomment-993538772
CI failed, please take a look:
[ERROR] org.apache.curator.framework.state.TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection
[ERROR] Run 1: TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection:110 NoSuchMethod
[ERROR] Run 2: TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection:110 NoSuchMethod
[ERROR] Run 3: TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection:110 NoSuchMethod
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] shikharid edited a comment on pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
shikharid edited a comment on pull request #401:
URL: https://github.com/apache/curator/pull/401#issuecomment-1027958706
Any plans to merge and release this?
And will this fix be backported to 2.xx release?
I recently faced this in some production servers, where after a network blip of couple of minutes all consumers of the queue stopped receiving any updates.
In case this won't be backported, I will have to create a fork and apply those changes for the time being.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] shikharid commented on pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
shikharid commented on pull request #401:
URL: https://github.com/apache/curator/pull/401#issuecomment-1027958706
Any plans to merge and release this?
And will this fix be backported to 2.xx release?
I recently faced this where a network blip of couple of minutes left all consumers of the queue stopped receiving any updates. In case this won't be backported, I will have to create a fork and apply those changes for the time being.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] faucct commented on a change in pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
faucct commented on a change in pull request #401:
URL: https://github.com/apache/curator/pull/401#discussion_r749038813
##########
File path: curator-recipes/src/main/java/org/apache/curator/framework/recipes/queue/ChildrenCache.java
##########
@@ -86,12 +101,14 @@ private Data(List<String> children, long version)
void start() throws Exception
{
- sync(true);
+ client.getConnectionStateListenable().addListener(connectionStateListener);
+ sync();
}
@Override
public void close() throws IOException
{
+ client.getConnectionStateListenable().removeListener(connectionStateListener);
Review comment:
Should not this be after `client.removeWatchers()`, in reverse order with `#start()`?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] Randgalt commented on pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
Randgalt commented on pull request #401:
URL: https://github.com/apache/curator/pull/401#issuecomment-994688485
> CI failed, please take a look:
>
> [ERROR] org.apache.curator.framework.state.TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection [ERROR] Run 1: TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection:110 NoSuchMethod [ERROR] Run 2: TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection:110 NoSuchMethod [ERROR] Run 3: TestConnectionStateManager.testConnectionStateRecoversFromUnexpectedExpiredConnection:110 NoSuchMethod
The tests pass locally when I run them. Curator has so much flakiness with testing. I'll re-run Travis to see what it shows.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] shikharid edited a comment on pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
shikharid edited a comment on pull request #401:
URL: https://github.com/apache/curator/pull/401#issuecomment-1027958706
Any plans to merge and release this?
And will this fix be backported to 2.xx release?
I recently faced this where after a network blip of couple of minutes all consumers of the queue stopped receiving any updates. In case this won't be backported, I will have to create a fork and apply those changes for the time being.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [curator] Randgalt commented on a change in pull request #401: CURATOR-623 - ChildrenCache (used by Queues) didn't have a ConnectionStateListener
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #401:
URL: https://github.com/apache/curator/pull/401#discussion_r749426038
##########
File path: curator-recipes/src/main/java/org/apache/curator/framework/recipes/queue/ChildrenCache.java
##########
@@ -66,6 +68,19 @@ public void processResult(CuratorFramework client, CuratorEvent event) throws Ex
}
};
+ private final ConnectionStateListener connectionStateListener = (__, newState) -> {
+ if ((newState == ConnectionState.CONNECTED) || (newState == ConnectionState.RECONNECTED)) {
+ try
+ {
+ sync();
+ }
+ catch ( Exception e )
+ {
+ throw new RuntimeException(e);
Review comment:
Yeah, it logs. It's not very obvious but these are handled by `MappingListenerManager` (see https://github.com/apache/curator/blob/master/curator-framework/src/main/java/org/apache/curator/framework/listen/MappingListenerManager.java#L85)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org