Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 327034

Summary: [remoteservices][osgi] fix RemoteServiceEndpointDescriptionImpl.equals
Product: [RT] ECF Reporter: Scott Lewis <slewis>
Component: ecf.remoteservicesAssignee: ecf.core-inbox <ecf.core-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: bhunt, bugs.eclipse.org
Version: 3.4.0   
Target Milestone: 3.4.0   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Bug Depends on:    
Bug Blocks: 327060    
Attachments:
Description Flags
proposed fix none

Description Scott Lewis CLA 2010-10-05 14:45:00 EDT 
The current implementation of RemoteServiceEndpointDescriptionImpl.equals uses to things to determine whether one endpoint description is equal to another:

a) the discovered service location (URI)...which only uniquely identifies the host
b) the remote service id (only unique on a container-specific basis)

The endpoint id should be added to this equality test, in order to distinguish the endpoints (container id) for the use cases where more than one remote service container exists within a given vm, or in a separate vm on a single host.
Comment 1 Scott Lewis CLA 2010-10-05 14:51:59 EDT 
Created attachment 180274 [details]
proposed fix

This fix adds the use of the endpointId to determine equality between endpoint description impl instances.  This means that when there is more than one remote service container on a given location, the use of endpointId will assure that the RemoteServiceEndpointDescriptionImpls will return false to equals.
Comment 2 Bryan Hunt CLA 2010-10-05 17:39:19 EDT 
Markus replied on the dev mailing list that the location should include the port number.  So is the bug really in the RemoteServiceEndpointDescriptionImpl, or is the bug really in the zookeeper AdversisedService where it sets the location to the host ip and does not include the container port?
Comment 3 Scott Lewis CLA 2010-10-05 18:04:09 EDT 
(In reply to comment #2)
> Markus replied on the dev mailing list that the location should include the
> port number.  So is the bug really in the RemoteServiceEndpointDescriptionImpl,
> or is the bug really in the zookeeper AdversisedService where it sets the
> location to the host ip and does not include the container port?

I think both.  That is, I think that the endpointId should be used to test for equality, because even *if* the port is added to the location (which I agree it should be, btw), the endpoint Id (at least for some providers) can have path information as well...and *that* can be different for a given endpoint, even if the hostname and port are different.

So I'm inclined to make this change (incorporate the use of endpointId for equals), AND have another bug opened to add the port info to the location.

BTW...just for everyone's information...the Remote Service Admin spec has an EndpointDescription super class, with an endpoint (as String), that is used (as it should be) in the equals test...so all the existing code here is likely to be changed soon...but for 3.4 having it fixed in RemoteServiceEndpointDescriptionImpl will make sense.
Comment 4 Scott Lewis CLA 2010-10-06 01:12:44 EDT 
Fix for this issue (equals) released to HEAD.  Bug 327060 opened to deal with
other issue described in comment 2 and comment 3.
Comment 5 Markus Kuppe CLA 2010-10-06 01:41:28 EDT 
(In reply to comment #3)
> BTW...just for everyone's information...the Remote Service Admin spec has an
> EndpointDescription super class, with an endpoint (as String), that is used (as
> it should be) in the equals test...so all the existing code here is likely to
> be changed soon...but for 3.4 having it fixed in
> RemoteServiceEndpointDescriptionImpl will make sense.

IMO equality should not be checked based on a single String, e.g. think about:

scheme://locahost:1234/sdkjvklk2342bc=
scheme://127.0.0.1:1234/sdkjvklk2342bc=
Comment 6 Scott Lewis CLA 2010-10-06 09:46:14 EDT 
(In reply to comment #5)
> (In reply to comment #3)
> > BTW...just for everyone's information...the Remote Service Admin spec has an
> > EndpointDescription super class, with an endpoint (as String), that is used (as
> > it should be) in the equals test...so all the existing code here is likely to
> > be changed soon...but for 3.4 having it fixed in
> > RemoteServiceEndpointDescriptionImpl will make sense.
> 
> IMO equality should not be checked based on a single String, e.g. think about:
> 
> scheme://locahost:1234/sdkjvklk2342bc=
> scheme://127.0.0.1:1234/sdkjvklk2342bc=

One nice thing about using ID.equals (which is being used now in patch in RemoteServiceEndpointDescriptionImpl) is that it isn't necessarily based upon String equality...i.e. ID.equals can treat the above as the same (if it's written to do so).  This allows some flexibility in equality testing for different providers/different ID types.

But the remote service admin spec has EndpointDescription equality based upon String endpoint id.  So it might be a good idea to make your point with whomever is currently working on the event admin spec for 4.3.  

If, however, we create a subclass of EndpointDescription to use for ECF descriptions we can use ID.equals rather than String equals for EndpointDescription equality testing in ECF...and I think this would be a wise thing to do.