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/06/15 14:39:04 UTC
[GitHub] [curator] eolivelli opened a new pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
eolivelli opened a new pull request #367:
URL: https://github.com/apache/curator/pull/367
----------------------------------------------------------------
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
[GitHub] [curator] itarakanov commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
itarakanov commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442344391
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -163,9 +164,19 @@ public void removeListener(ServiceCacheListener listener)
discovery.getClient().getConnectionStateListenable().removeListener(listener);
}
+ private boolean isRootPathEvent(PathChildrenCacheEvent event) {
Review comment:
I would rather call it something like **isEmptyPathEvent**, because "root" is a relative thing.
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442275526
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
I see this problem with ZK 3.5.7 not with 3.6.
btw @Randgalt I see that CuratorCacheListener does not hold a reference to the root path, and it does not hold any reference to the CuratorCache.
I cannot introduce such reference, because it will be an incompatible change.
Maybe the problem is that PathChildrenCacheListenerWrapper cannot receive the reference to the path.
I can add some new listener callback to CuratorCacheListener in order to let it receive the base path
interface CuratorCacheListener {
....
default void configureRootPath(String path) {}
}
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442275526
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
I see this problem with ZK 3.5.7 not with 3.6.
btw @Randgalt I see that CuratorCacheListener does not hold a reference to the root path, and it does not hold any reference to the CuratorCache.
I cannot introduce such reference, because it will be an incompatible 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
[GitHub] [curator] eolivelli commented on pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #367:
URL: https://github.com/apache/curator/pull/367#issuecomment-648086636
@Randgalt I used the GitHub button and this resulted with a merge commit on git history.
I apologize.
----------------------------------------------------------------
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
[GitHub] [curator] itarakanov commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
itarakanov commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442344391
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -163,9 +164,19 @@ public void removeListener(ServiceCacheListener listener)
discovery.getClient().getConnectionStateListenable().removeListener(listener);
}
+ private boolean isRootPathEvent(PathChildrenCacheEvent event) {
Review comment:
I would rather call it something like **isEmptyPathEvent**, because "root" is a relative thing.
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r440971288
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
If pushing it down fixes it then it is a regression. I'm surprised we don't have a test for this. We can get a new version out if needed.
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r440969485
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
@Randgalt do you think this is a regression ?
I will push down the fix to CuratorCacheListener, thanks for the suggestion.
I didn't go that way because I wasn't sure.
It is important to understand if this is a regression in 5.0 and then we have to fix it asap, otherwise applications will break
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442314675
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
@eolivelli I think it could be handled by adding a new `CuratorCache.Options` - something like `IGNORE_ROOT` and then `CuratorCacheBridgeBuilder` would always set that option.
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442279406
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
`I'm surprised we don't have a test for this`
If you run tests you will see the error in the logs, but the error is swallowed
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli commented on pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #367:
URL: https://github.com/apache/curator/pull/367#issuecomment-647356586
@Randgalt
the patch is finally ready for review.
I have added a few assertions on an existing test.
With this fix the errors on CURATOR-574 are not present anymore.
I wonder if it is better to rename the issue, actually this was a problem on PathChildrenCacheListenerWrapper.java and not in ServiceDiscovery.
The change is not backward compatibile, but it is on a fresh new API of Curator 5.0, so if you release 5.0.1 (or 5.1 if you want to strictly observe SemVer) it won't be a big deal
I can create the 5.0.1 release
----------------------------------------------------------------
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
[GitHub] [curator] itarakanov commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
itarakanov commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442283531
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
We can see the problem even in ZK 3.6.1
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r440967668
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
The bug might actually be in `forPathChildrenCache()` of `CuratorCacheListener`. We shouldn't have to filter the root out. I believe PathChildrenCache didn't report on the root so `forPathChildrenCache` should emulate that 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
[GitHub] [curator] Randgalt commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r440971288
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
If pushing it down fixes it then it is a regression. I'm surprised we don't have a test for this. We can get a new version out if needed. However, it will only be an issue with ZK 3.6 - pre-3.6 PathChildrenCache is used.
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
Randgalt commented on pull request #367:
URL: https://github.com/apache/curator/pull/367#issuecomment-647531682
> I can create the 5.0.1 release
Please do
----------------------------------------------------------------
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
[GitHub] [curator] eolivelli merged pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #367:
URL: https://github.com/apache/curator/pull/367
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on a change in pull request #367: CURATOR-574 DiscoveryService fatal error on deserializing an empty byte[] as JSON
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #367:
URL: https://github.com/apache/curator/pull/367#discussion_r442312520
##########
File path: curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceCacheImpl.java
##########
@@ -78,7 +79,7 @@ private static ExecutorService convertThreadFactory(ThreadFactory threadFactory)
this.discovery = discovery;
- String path = discovery.pathForName(name);
+ path = discovery.pathForName(name);
Review comment:
> I see this problem with ZK 3.5.7 not with 3.6.
Yeah - I'm mistaken. I thought it use PathChildrenCache in pre-3.6 but it uses TreeCache which will have the same problem.
----------------------------------------------------------------
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