[
https://issues.apache.org/jira/browse/CURATOR-164?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14506027#comment-14506027
]
Mike Drob commented on CURATOR-164:
-----------------------------------
I'll post review comments here so that I can batch them instead of triggering
notifications for each line.
bq. Holder.java
I'd like to see some JavaDoc on the methods. At a minimum on
{{getServiceIfRegistered}} warning that it can return {{null}}.
Also, if every method starts with {{lock.lock()}} and ends with
{{lock.unlock()}} then they might as well be synchronized, and drop the lock
entirely. When you get the lock and use it, (like in reRegisterServices) you
can just synchronize on the Holder instance directly.
{code}
+ long elapsed = System.currentTimeMillis() - stateChangeMs;
+ if ( elapsed >= cleanThresholdMs )
+ {
+ return true;
+ }
{code}
Using {{currentTimeMillis}} is a time bomb.
http://stackoverflow.com/questions/2978598/will-sytem-currenttimemillis-always-return-a-value-previous-calls/2979239#2979239
Please avoid unrelated whitespace changes - they make the patch harder to
review.
In general, I am not a fan of the eventual consistency provided by clean for
this. Once we are down to one map, why can't we clean it immediately instead of
waiting for things to expire?
> curator-x-discovery: unregisterService is not guaranteed to remove the
> service, due to reconnectListener concurrency issue
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: CURATOR-164
> URL: https://issues.apache.org/jira/browse/CURATOR-164
> Project: Apache Curator
> Issue Type: Bug
> Components: Framework
> Affects Versions: 2.7.0
> Reporter: Rasmus Berg Palm
> Assignee: Jordan Zimmerman
> Priority: Critical
> Fix For: 2.8.0
>
>
> In ServiceDiscoveryImpl:
> When unregistering a service, the reconnect listener might fire while
> deleting the path.
> This can cause a condition where the delete finishes successfully, the
> service is removed from services, and then the reRegisterServices completes
> successfully and the service is added back in ZK and in services, end result
> being that the service was not removed, even though unregisterService did not
> throw any exceptions.
> Essentially the use of the internal 'services' cache makes for a nightmare of
> concurrency issues. I put this as critical as the library it's really not
> usable IMO.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
