Curator 94 #2
Curator 94 #2
Conversation
…rations are not stored within the service. This is to prevent two side effects: When the ServiceDiscoveryImpl class is closed all PERMANENT registrations are deleted. This is not desireable since callers who create PERMANENT registations should expect them to survive until explicitly deleted. PERMANENT registrations should be stateless with respect to a particular service discovery client (assume a cluster of rest services). It should be possible to use any of them to register/unregister. It is not desirable to keep re-registering them whenever the client losses/reconnects to Zookeeper.
There was a problem hiding this comment.
This will prevent updateService for permanent services. Is that desirable?
There was a problem hiding this comment.
My thinking here was that a permanent service should extend beyond the scope of the client used to register it (either directly or via the rest service) - what is the definition of "permanent" otherwise, therefore details of the service would be lost (not in memory) when the client was restarted anyway.
To avoid two different modes (registering client verses later client) I decided that no client should keep a record of it.
The alternative would have been to have all clients (watch zookeeper and) load permanent registrations from Zookeeper.
From a design perspective I see this alternative as perfectly reasonable too, its just its a bunch more work and still represents a change in current functionality.
What are your thoughts?
|
First of all thank you for this PR, I was looking into some other issues with permanent service types and this would certainly have been a big problem as I did expect permanent service types to last beyond the lifetime of Curator's service discovery. A slightly related question (I hope), looking at your diff you don't seem to be doing anything for the unregistration of these services. Looking at https://github.com/apache/curator/blob/master/curator-x-discovery/src/main/java/org/apache/curator/x/discovery/details/ServiceDiscoveryImpl.java#L241 Note: As a user, I think not being able to update permanent servicetypes could easily be worked around by unregistering and reregistering them with the new information. |
|
I am afraid that this patch is more than a year old and the underlying code has changed. I believe the original code with the patch applied looked like this: Hence your suggestion of unregistered and re-registering would have worked, however as you have pointed out, this approach will not work any more as unregistration now requires that the service be in the map. I have not looked at this issue since the patch was submitted (it worked for me ;-) however a very cursory scan of the current code base didn't indicate the the core issue had been fixed (PERMANENT services are not permanent). I don't really like my original suggestion of loading PERMANENT registrations into all clients, as it would require a lot more overhead to monitor (watch) and load registrations in other clients, a simpler solution would be to add special cases to the update and unregister methods so that they load the PERMANENT record from zookeper instead of using the map, however I suspect that this might introduce some nasty race conditions - frankly I would be fine with this and resolve the issue with documentation ... "PERMANENT registrations require external synchronization", however I can see why others may feel that isn't clean. I would also recommend confirming if the bug still exists and if so, removing PERMANENT until such time as someone creates a workable patch... otherwise the code base is just leaving a landmine for someone else to step on .... |
|
I'm having the same problem. Tried with the old 2.10.0 and the latest 4.0.0, but I'm still losing the PERMANENT service registration when I call ServiceDiscovery.close() method. Are there any news? Thanks |
|
Curator is now at Apache. Please open an issue on Curator's issue tracker: http://issues.apache.org/jira/browse/CURATOR |
|
Closed as stale. There're many changes since this patch was made in the first place. Feel free to resubmit it if it's still relevant. |
5 participants
https://issues.apache.org/jira/browse/CURATOR-94
Added some docs (based on my understanding) for ServiceType
Updated ServiceDiscoveryImpl.registerService so that PERMANENT registrations are not stored within the service.
This is to prevent two side effects:
When the ServiceDiscoveryImpl class is closed all PERMANENT registrations are deleted.
This is not desireable since callers who create PERMANENT registations should expect them to survive until explicitly deleted.
PERMANENT registrations should be stateless with respect to a particular service discovery client (assume a cluster of rest services).
It should be possible to use any of them to register/unregister.
It is not desirable to keep re-registering them whenever the client losses/reconnects to Zookeeper.