Bugzilla – Full Text Bug Listing
| Summary: | [Discovery][ZooDiscovery] ClassCastException in DiscoverdService.createServiceProperties | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [RT] ECF | Reporter: | Scott Lewis <slewis> | ||||
| Component: | ecf.discovery | Assignee: | Wim Jongman <wim.jongman> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | critical | ||||||
| Priority: | P3 | CC: | ahmed.aadel, bugs.eclipse.org, mpetzold, wim.jongman | ||||
| Version: | 3.5.0 | ||||||
| Target Milestone: | 3.5.0 | ||||||
| Hardware: | PC | ||||||
| OS: | Windows XP | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 324215 | ||||||
| Attachments: |
|
||||||
Raising this to critical...because it effectively makes the zookeeper discovery provider unable to work properly with the Remote Service Admin specification (since the RSA spec requires objectClass in EndpointDescription properties, and the current zookeeper provider changes/removes any values from the discovery properties that has 'objectClass' as key. I am able to get things to working properly by changing the behavior of DiscoverdService (to not remove objectClass values), AdvertisedService and NodeReader (to not change the value of the property with key 'objectClass')...would you like these fixes/files attached to this bug? (In reply to comment #1) > Raising this to critical...because it effectively makes the zookeeper discovery > provider unable to work properly with the Remote Service Admin specification > (since the RSA spec requires objectClass in EndpointDescription properties, and > the current zookeeper provider changes/removes any values from the discovery > properties that has 'objectClass' as key. > > I am able to get things to working properly by changing the behavior of > DiscoverdService (to not remove objectClass values), AdvertisedService and > NodeReader (to not change the value of the property with key > 'objectClass')...would you like these fixes/files attached to this bug? Removal in DiscoverdService occurs on zooDiscovery internal properties. that is, the map with own properties keys that zooDiscovery instances exchange via the undelying zookeeper. Once a service is discoverered, internal properties are cleared/swapped by original ones. In this case there an error using keys. I think this would fix it: swap this: String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS); if (services == null) { services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); } by this: String[] services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); if (services == null) { propMap.put(Constants.OBJECTCLASS,services); } ...oops..the second "if (services == null)" should be "if (services != null)". (In reply to comment #2) > swap this: > > String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS); > if (services == null) { > services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); > } > > by this: > > String[] services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); > if (services == null) { > propMap.put(Constants.OBJECTCLASS,services); > } In addition to this logic (with !=) I think the advertiser and nodereader also have to be prevented from setting (or resetting) the OBJECTCLASS property (it can set the NODE_PROPERTY_SERVICES of course but I think as of now it does both). The main thing is that if the OBJECTCLASS property is set (by the zoodiscovery client), then the zoodiscovery implementation shouldn't somehow change the OBJECTCLASS property value. Currently, what's happening is that the OBJECTCLASS value is effectively getting changed by zoodiscovery. adding block of rsa impl service properties are sent from one discovery instance to another as binaries through the underlying zookeeper. The issue was in not using the right internal key to recover the incoming property (in this case service types which are values mapped to key Constants.OBJECTCLASS, internally send under INode.NODE_PROPERTY_SERVICES property key for design/implementation reasons). If the fix I offered fixes this bug. Please do apply it (as I cannot commit it myself) and consider this bug closed. Created attachment 187760 [details] proposed fixes for DiscoverdService, AdvertisedService, NodeReader This attachment is a zip file with three files changed from org.eclipse.ecf.provider.zookeeper: DiscoverdService.java, AdvertisedService.java, and NodeReader.java. These work with the AdvertisedService(IServiceInfo) constructor, which is the only constructor (as far as I can tell) that is used by the zookeeper provider. I don't have the means to test AdvertisedService(ServiceReference). The basic strategy that for the AdversisedService(IServiceInfo) constructor, the NODE_PROPERTY_NAME_SERVICES is set (to services String[]). For the AdvertisedService(ServiceReference) constructor the NODE_PROPERTY_NAME_SERVICES is *not* set, and rather the OBJECTCLASS is set. Alternatively, on the NodeReader and DiscoverdService, if the NODE_PROPERTY_NAME_SERVICES is not set, then the OBJECTCLASS value is removed. Also includes changes to deal with bug 335483, is that code was intermixed with the code changes needed to address this bug. Please review the changes, and if they look ok let me know and I will release them to master. (In reply to comment #7) > Created attachment 187760 [details] > proposed fixes for DiscoverdService, AdvertisedService, NodeReader > > This attachment is a zip file with three files changed from > org.eclipse.ecf.provider.zookeeper: DiscoverdService.java, > AdvertisedService.java, and NodeReader.java. > > These work with the AdvertisedService(IServiceInfo) constructor, which is the > only constructor (as far as I can tell) that is used by the zookeeper provider. > I don't have the means to test AdvertisedService(ServiceReference). > > The basic strategy that for the AdversisedService(IServiceInfo) constructor, > the NODE_PROPERTY_NAME_SERVICES is set (to services String[]). For the > AdvertisedService(ServiceReference) constructor the NODE_PROPERTY_NAME_SERVICES > is *not* set, and rather the OBJECTCLASS is set. > > Alternatively, on the NodeReader and DiscoverdService, if the > NODE_PROPERTY_NAME_SERVICES is not set, then the OBJECTCLASS value is removed. > > Also includes changes to deal with bug 335483, is that code was intermixed with > the code changes needed to address this bug. > > Please review the changes, and if they look ok let me know and I will release > them to master. For the this bug there is no need to change anything except the swap I mentioned before, this one: > String[] services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); > if (services =! null) { > propMap.put(Constants.OBJECTCLASS,services); > } I think this solves the issue this bug is about keeping the integrity of zooDiscovery. I'll comment on the other bug in its very place once I have time for that. (In reply to comment #8) <stuff deleted> > > For the this bug there is no need to change anything except the swap I > mentioned before, this one: > > > String[] services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); > > if (services =! null) { > > propMap.put(Constants.OBJECTCLASS,services); > > } > > I think this solves the issue this bug is about keeping the integrity of > zooDiscovery. No, it doesn't. If only this change is made, and there is an OBJECTCLASS key in the properties (as there is with remote service admin), then Zoodiscovery fails with an exception...because the type is changed by the Zoodiscovery messing with/changing the values of the properties. In general, the properties specified externally (i.e. those passed in via IServiceInfo) should never be messed with/changed. Otherwise it's a violation of the discovery API contract...and zoodiscovery currently violates that contract. Are we talking about the same exception which is the origin of this bug? Otherwise can you please post its stack trace? (In reply to comment #10) > Are we talking about the same exception which is the origin of this bug? > Otherwise can you please post its stack trace? The stack trace is long gone unfortunately, but I believe it was a ClassCastException in NodeReader.processResult. The reason was that the NodeReader was trying to cast the OBJECTCLASS value to a String[]...and that fails...because the value was set (by RSA code) outside of zoodiscovery. The basic problem was/is that zoodiscovery (or any discovery provider) should not be reading/changing/setting *any* property values that are not strictly internal (i.e. NODE_PROPERTY_NAME_*). Changing the value of any property that can/is specified externally (e.g. OBJECTCLASS in the case of RSA), violates the basic contract (because receivers don't get the same values that they specified). The fixes attached put in place a strategy to allow the fussing with OBJECTCLASS because of the AdvertisedService(ServiceReference) constructor. I would prefer to have this constructor completely removed, but since that effects other aspects of the zoodiscovery provider that I don't want to mess with, I put in place this 'dual strategy' (use NODE_PROPERTY_NAME_SERVICES with AdvertisedService(IServiceInfo) constructor, and OBJECTCLASS with AdvertisedService(ServiceReference) constructor). Are we talking about the same exception which is the origin of this bug? Otherwise can you please post its stack trace?(In reply to comment #11) > (In reply to comment #10) > > Are we talking about the same exception which is the origin of this bug? > > Otherwise can you please post its stack trace? > > The stack trace is long gone unfortunately, but I believe it was a > ClassCastException in NodeReader.processResult. The reason was that the > NodeReader was trying to cast the OBJECTCLASS value to a String[]...and that > fails...because the value was set (by RSA code) outside of zoodiscovery. > > The basic problem was/is that zoodiscovery (or any discovery provider) should > not be reading/changing/setting *any* property values that are not strictly > internal (i.e. NODE_PROPERTY_NAME_*). Changing the value of any property that > can/is specified externally (e.g. OBJECTCLASS in the case of RSA), violates the > basic contract (because receivers don't get the same values that they > specified). > > The fixes attached put in place a strategy to allow the fussing with > OBJECTCLASS because of the AdvertisedService(ServiceReference) constructor. I > would prefer to have this constructor completely removed, but since that > effects other aspects of the zoodiscovery provider that I don't want to mess > with, I put in place this 'dual strategy' (use NODE_PROPERTY_NAME_SERVICES with > AdvertisedService(IServiceInfo) constructor, and OBJECTCLASS with > AdvertisedService(ServiceReference) constructor). The thing I'm trying to explain is that properties of a service being advertised are read and put in a zookeeper node as bytes, then comes out in the (remote) end, read and put back with their original key-value format. How do you intend to distribute properties of service without reading them? How do you intend to reconstruct properties without putting them back? There is no violation or mess up as you prefer to call it. What you are suggesting is a solution bigger than the problem itself which would require a deep check of zooDiscovery integrity. I'm trying to keep the change sizable to the time I can afford as much as possible and yet solve the current problem? Would you please put the mentioned stack trace for the bugs integrity sake? (In reply to comment #12) > Are we talking about the same exception which is the origin of this bug? > Otherwise can you please post its stack trace?(In reply to comment #11) > > (In reply to comment #10) > > > Are we talking about the same exception which is the origin of this bug? > > > Otherwise can you please post its stack trace? No, it's not the same ClassCastException. It was a ClassCastException in NodeReader.java. Ahmed it's a major hassle for me to reproduce the exception/stack trace, but I can tell you it was a ClassCastException in NodeReader.java, line 81. <stuff deleted> > > The thing I'm trying to explain is that properties of a service being > advertised are read and put in a zookeeper node as bytes, then comes out in the > (remote) end, read and put back with their original key-value format. How do > you intend to distribute properties of service without reading them? This doesn't make any sense to me Ahmed. Of course the properties are read...it's just that their type/values aren't *changed* by zoodiscovery. >How do you > intend to reconstruct properties without putting them back? Again, it's not a matter of reading them or putting them back (to transmit them). It's that OBJECTCLASS value *SHOULD NOT BE CHANGED* by Zoodiscovery (because it should be set to whatever the client wants it to be set to). Currently, Zoodiscovery *assumes* that OBJECTCLASS should/will be String[]. It should not assume that, or change the value that is passed in for OBJECTCLASS. It should simply pass it through like it does all other properties. With the old code it did change it. >There is no > violation or mess up as you prefer to call it. The paragraph above describes a violation of the discovery API contract...i.e. that the discovery provider will pass through unchanged all properties that are passed in via IServiceInfo. As it was, Zoodiscovery changed the OBJECTCLASS property value. The changes made were to address this problem...i.e. so that if the advertiser set OBJECTCLASS to a String (say), zoodiscovery wouldn't fail. >What you are suggesting is a > solution bigger than the problem itself which would require a deep check of > zooDiscovery integrity. No, that's not what I'm suggesting. What I'm suggesting is that zoodiscovery *not* change the OBJECTCLASS property in the transmission of discovery properties (at least when the AdvertisedService(IServiceInfo) constructor is used). >I'm trying to keep the change sizable to the time I can > afford as much as possible and yet solve the current problem? Yes...so maybe you could examine/review the attached source, as they do solve this problem. >Would you please > put the mentioned stack trace for the bugs integrity sake? ClassCastException (type of value incorrect) line 81 NodeReader.java (In reply to comment #13) > (In reply to comment #12) > > Are we talking about the same exception which is the origin of this bug? > > Otherwise can you please post its stack trace?(In reply to comment #11) > > > (In reply to comment #10) > > > > Are we talking about the same exception which is the origin of this bug? > > > > Otherwise can you please post its stack trace? > > No, it's not the same ClassCastException. It was a ClassCastException in > NodeReader.java. > > Ahmed it's a major hassle for me to reproduce the exception/stack trace, but I > can tell you it was a ClassCastException in NodeReader.java, line 81. > > <stuff deleted> > > > > The thing I'm trying to explain is that properties of a service being > > advertised are read and put in a zookeeper node as bytes, then comes out in the > > (remote) end, read and put back with their original key-value format. How do > > you intend to distribute properties of service without reading them? > > This doesn't make any sense to me Ahmed. Of course the properties are > read...it's just that their type/values aren't *changed* by zoodiscovery. > > >How do you > > intend to reconstruct properties without putting them back? > > Again, it's not a matter of reading them or putting them back (to transmit > them). It's that OBJECTCLASS value *SHOULD NOT BE CHANGED* by Zoodiscovery > (because it should be set to whatever the client wants it to be set to). > Currently, Zoodiscovery *assumes* that OBJECTCLASS should/will be String[]. It > should not assume that, or change the value that is passed in for OBJECTCLASS. > It should simply pass it through like it does all other properties. With the > old code it did change it. > > >There is no > > violation or mess up as you prefer to call it. > > The paragraph above describes a violation of the discovery API contract...i.e. > that the discovery provider will pass through unchanged all properties that are > passed in via IServiceInfo. As it was, Zoodiscovery changed the OBJECTCLASS > property value. The changes made were to address this problem...i.e. so that > if the advertiser set OBJECTCLASS to a String (say), zoodiscovery wouldn't > fail. > > >What you are suggesting is a > > solution bigger than the problem itself which would require a deep check of > > zooDiscovery integrity. > > No, that's not what I'm suggesting. What I'm suggesting is that zoodiscovery > *not* change the OBJECTCLASS property in the transmission of discovery > properties (at least when the AdvertisedService(IServiceInfo) constructor is > used). > > >I'm trying to keep the change sizable to the time I can > > afford as much as possible and yet solve the current problem? > > Yes...so maybe you could examine/review the attached source, as they do solve > this problem. > > >Would you please > > put the mentioned stack trace for the bugs integrity sake? > > ClassCastException (type of value incorrect) line 81 NodeReader.java > This doesn't make any sense to me Ahmed. Of course the properties are > read...it's just that their type/values aren't *changed* by zoodiscovery. This makes sense replying to "(reading/<..>/setting)" in this: <-- <omitted stuff> The basic problem was/is that zoodiscovery (or any discovery provider) should> not be reading/changing/setting *any* property <omitted stuff> --> > Again, it's not a matter of reading them or putting them back (to transmit > them). It's that OBJECTCLASS value *SHOULD NOT BE CHANGED* by Zoodiscovery > (because it should be set to whatever the client wants it to be set to). > Currently, Zoodiscovery *assumes* that OBJECTCLASS should/will be String[]. Per OSGi specs, its value must be of type String[]: Core specs 4.2/6.1.13.67 <specs> public static final String OBJECTCLASS = “objectClass” Service property identifying all of the class names under which a service was registered in the Framework. The value of this property must be of type String[]. This property is set by the Framework when a service is registered. </specs> >No, that's not what I'm suggesting. What I'm suggesting is that zoodiscovery >*not* change the OBJECTCLASS property in the transmission of discovery >properties (at least when the AdvertisedService(IServiceInfo) constructor is >used). Changing NodeReader.java implies changing, at least, NodeWriter.java which is the object writing down to the underlying zooKeeper. Being written values must be serializable and type-recognizable once deserialized in the other end point. So, this cascading change touches the core of zooDiscovery and require checking much of the code for integrity. Up to now, I don't see yet any need to go that far. >Yes...so maybe you could examine/review the attached source, as they do solve >this problem. Given the little info available here (>ClassCastException (type of value incorrect) line 81 NodeReader.java), I cannot make an informed review. Anyway, if the solution you suggested solves the problem and you feel confident about it, please do apply it. > Per OSGi specs, its value must be of type String[]:
> Core specs 4.2/6.1.13.67
> <specs>
> public static final String OBJECTCLASS = “objectClass”
> Service property identifying all of the class names under which a service
> was registered in the Framework. The value of this property must be of type
> String[].
> This property is set by the Framework when a service is registered.
> </specs>
ECF discovery API and OSGi specs are two distinct APIs. While the discovery API gets used inside the scope of OSGi, you might also use it outside of OSGi and still name a key/value pair OBJECTCLASS=something
Bottom line, the OSGi spec does not apply to ECF discovery!
Another possibility to address this issue is the following. I would like feedback from the zookeeper provider implementers (i.e. those that implemented the bundle org.eclipse.ecf.provider.zookeeper) on this strategy. There are currently two constructors to org.eclipse.ecf.provider.zookeeper.core.AdvertisedService: public AdvertisedService(IServiceInfo) and public AdvertisedService(ServiceReference) The first one is used by the provider to implement the discovery API IDiscoveryLocator.registerService(IServiceInfo serviceInfo). It copies the properties from the IServiceInfo service properties, and puts the services into NODE_PROPERTY_NAME_SERVICES (an zookeeper-provider internal property). The second one (AdvertisedService(ServiceReference)) is the one that creates/introduces the problem associated this bug. The reason it's problematic is because it accesses the OBJECTCLASS property in the ServiceReference.getProperty, and copies the String[] value of the OBJECTCLASS property directly into the internal service properties (for transmission)...after converting it to a String (arrayToString I believe). So one solution to this whole issue would be to remove/delete the AdvertisedService(ServiceRefeference) constructor. As far as I can tell, the only actual usage of AdvertisedService(ServiceReference) is in org.eclipse.ecf.provider.zookeeper.node.internal.WatchManager.publish(ServiceReference) and the only reference to publish(ServiceReference) is in org.eclipse.ecf.provider.zookeeper.node.internal.WatchManager.update(ServiceReference) None of these methods (WatchManager.update, WatchManager.publish, AdvertisedService(ServiceReference)) are part of the discovery API, and if I delete them, everything in ECF compiles. So one possibility is that these three methods be deleted, and then *all* references to OBJECTCLASS would be gone from the provider...and NODE_PROPERTY_NAME_SERVICES could be the *only* property that's used in NodeReader, DiscoverdService, and AdvertisedService. Thus the solution to this bug would be much simpler...always use NODE_PROERTY_NAME_SERVICES...and this would also simpify the behavior of the zookeeper provider significantly. Comments about this approach (simplifying by removing AdvertisedService(ServiceReference)? Is this problematic for anyone? One thing to keep in mind: If it's problematic because some clients were previously using AdvertisedService(ServiceReference), then it may be better to use the ECF RSA implementation...which provides a standard way to publish/discover EndpointDescriptions (meta-data about remote services). One other question/thought: why aren't org.eclipse.ecf.provider.zookeeper, org.eclipse.ecf.provider.zookeeper.core, and org.eclipse.ecf.provider.zookeeper.util internal packages? Is it because of IDiscoveryConfig?...or something else related to configuration? Even though I'm not one of the authors of Zoodiscovery, I'm +1 for eliminating the dead code. Actually I stumbled over this myself, but unfortunately had no time to work on it immediately. When doing this, the major version should be incremented to indicate breaking API changes. (In reply to comment #16) > Another possibility to address this issue is the following. I would like > feedback from the zookeeper provider implementers (i.e. those that implemented > the bundle org.eclipse.ecf.provider.zookeeper) on this strategy. > > There are currently two constructors to > org.eclipse.ecf.provider.zookeeper.core.AdvertisedService: > > public AdvertisedService(IServiceInfo) > > and > > public AdvertisedService(ServiceReference) > > The first one is used by the provider to implement the discovery API > IDiscoveryLocator.registerService(IServiceInfo serviceInfo). It copies the > properties from the IServiceInfo service properties, and puts the services into > NODE_PROPERTY_NAME_SERVICES (an zookeeper-provider internal property). > > The second one (AdvertisedService(ServiceReference)) is the one that > creates/introduces the problem associated this bug. The reason it's > problematic is because it accesses the OBJECTCLASS property in the > ServiceReference.getProperty, and copies the String[] value of the OBJECTCLASS > property directly into the internal service properties (for > transmission)...after converting it to a String (arrayToString I believe). > > So one solution to this whole issue would be to remove/delete the > AdvertisedService(ServiceRefeference) constructor. As far as I can tell, the > only actual usage of AdvertisedService(ServiceReference) is in > > org.eclipse.ecf.provider.zookeeper.node.internal.WatchManager.publish(ServiceReference) > > and the only reference to publish(ServiceReference) is in > > org.eclipse.ecf.provider.zookeeper.node.internal.WatchManager.update(ServiceReference) > > None of these methods (WatchManager.update, WatchManager.publish, > AdvertisedService(ServiceReference)) are part of the discovery API, and if I > delete them, everything in ECF compiles. > > So one possibility is that these three methods be deleted, and then *all* > references to OBJECTCLASS would be gone from the provider...and > NODE_PROPERTY_NAME_SERVICES could be the *only* property that's used in > NodeReader, DiscoverdService, and AdvertisedService. Thus the solution to this > bug would be much simpler...always use NODE_PROERTY_NAME_SERVICES...and this > would also simpify the behavior of the zookeeper provider significantly. > > Comments about this approach (simplifying by removing > AdvertisedService(ServiceReference)? Is this problematic for anyone? One > thing to keep in mind: If it's problematic because some clients were > previously using AdvertisedService(ServiceReference), then it may be better to > use the ECF RSA implementation...which provides a standard way to > publish/discover EndpointDescriptions (meta-data about remote services). > > One other question/thought: why aren't org.eclipse.ecf.provider.zookeeper, > org.eclipse.ecf.provider.zookeeper.core, and > org.eclipse.ecf.provider.zookeeper.util internal packages? Is it because of > IDiscoveryConfig?...or something else related to configuration? I'd keep public AdvertisedService(ServiceReference). This is how ZooDiscovery used to work before open-sourcing it to ECF. We still use service references to publish our services in our own project using zooDiscovery as we're (in addition to loose coupling with discovery) distribution independent, be it Riena, CXF or r_osgi, restlet-based...We automate discovery and distribution without necessarily using ServiceInfo's types or ECF distribution internals in our code side, for long term design choices. I think ECF Discovery API is implemented, that's what matter most in this aspect. It is also mentioned in (http://wiki.eclipse.org/Zoodiscovery) as such, should someone want to avoid manually building a ServiceInfo object if they already have a service reference in hand, which is more likely. It's not so that it should be kept because we (the zooDiscovery issuers) use it and works just fine, but because our case could be other's, hence this option. All what deals directly with the underlying zooKeeper is marked as internal for the obvious reason it's too sensitive code and has "nothing" to do with the wrapping api. (In reply to comment #16) > [...] > > So one solution to this whole issue would be to remove/delete the > AdvertisedService(ServiceRefeference) constructor. [...] > So one possibility is that these three methods be deleted, and then *all* > references to OBJECTCLASS would be gone from the provider The removal of the contructor and the two methods does *not* work out for me. There is still (for me the same as before) Exception in 'DiscoverdService': ----- KEY: node.property.name.services [12.02.2011 12:12:58][org.apache.zookeeper.ClientCnxn] ERROR: Caught unexpected throwable java.lang.ClassCastException: [Ljava.lang.String; cannot be cast to java.lang.String at org.eclipse.ecf.provider.zookeeper.core.DiscoverdService.createServiceProperties(DiscoverdService.java:71) at org.eclipse.ecf.provider.zookeeper.core.DiscoverdService.<init>(DiscoverdService.java:57) at org.eclipse.ecf.provider.zookeeper.node.internal.NodeReader.processResult(NodeReader.java:100) at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:527) ----- This is because of class cast to String in 'createServiceProperties' of 'DiscoverdService'. The fix of Ahmed does not work out. ----- private ServiceProperties createServiceProperties(Properties props) { ServiceProperties result = new ServiceProperties(); for (Object k : props.keySet()) { System.out.println("KEY: " + k); Object value = (String) props.get(k); if (((String) k).startsWith(INode._BYTES_)) { result.setPropertyBytes(((String) k).split(INode._BYTES_)[1], (value + "").getBytes()); continue; } result.setProperty((String) k, value); } this.props = result.asProperties(); return result; } Just to know, what is exactly the modification in RSA to regular RS impl. in ECF 3.4 that causes this problem? So I can follow this in the code... Hi Martin, can you attach the complete stack trace? Is this the same one Scott could not reproduce? Hi Wim, this is my full stack trace for this. It comes up with r-osgi+zoodicovery on consumer end(!) with all bundles from master. I just register a regular remote service (provider end), nothing special in this case ;). In 'DiscoverdService.createServiceProperties(Properties props)' all propeties are handled as String and transformed to bytes. This however does not work out with String[] of the 'node.property.name.services' property. Adding an else with a removal of the property solves the problem, no Exception. However my tracker still does not trigger...so is this property used somewhere?
String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS);
if (services == null) {
services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES);
} else {
propMap.remove(INode.NODE_PROPERTY_SERVICES);
}
I believe what Martin is experiencing is exactly what my general issue is with things as they are (and probably discovered in the same way...i.e. by trying to use/test the RSA implementation). The core reason is that RSA sets a discovery property with name 'objectClass' (i.e. same as OSGI OBJECTCLASS constant) to a String, and the zookeeper discovery provider (both the advertiser and nodereader) make the assumption that the objectClass discovery property is a String[]. So this is the main thing that needs to be fixed (clients should be able to set a discovery property with name 'objectClass' to any type that they want...and it should be received with the same type and value as what was set on publish. I saw that Wim has marked bug 335483 as fixed...I haven't looked at that fix yet, but it's quite possible that the fix of that but fixes this bug as well. Wim will have to advise whether this is the case. (In reply to comment #23) > Adding an else with a removal of the property solves the problem, no Exception. > However my tracker still does not trigger...so is this property used somewhere? > > String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS); > if (services == null) { > services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); > } else { > propMap.remove(INode.NODE_PROPERTY_SERVICES); > } Only this does not work out. The Service endpoints are not handled even though they get discovered. The EndpointDesciptionLocater.matchServiceID returns false. "devsosgi.node.INode" (<- should be okay, these are the services!) Does not match: String org.eclipse.ecf.osgi.services.remoteserviceadmin.RemoteConstants.SERVICE_TYPE = "osgirsvc" Scott: Is this about RSA? ----- EndpointDesciptionLocater, line 807 ----- private boolean matchServiceID(IServiceID serviceId) { if (Arrays.asList(serviceId.getServiceTypeID().getServices()) .contains(RemoteConstants.SERVICE_TYPE)) return true; return false; } ----- (In reply to comment #25) > (In reply to comment #23) > > Adding an else with a removal of the property solves the problem, no Exception. > > However my tracker still does not trigger...so is this property used somewhere? > > > > String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS); > > if (services == null) { > > services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); > > } else { > > propMap.remove(INode.NODE_PROPERTY_SERVICES); > > } > > Only this does not work out. The Service endpoints are not handled even though > they get discovered. The EndpointDesciptionLocater.matchServiceID returns > false. > > "devsosgi.node.INode" (<- should be okay, these are the services!) > > Does not match: > > String > org.eclipse.ecf.osgi.services.remoteserviceadmin.RemoteConstants.SERVICE_TYPE = > "osgirsvc" > > Scott: Is this about RSA? No, it's not about RSA. The Zookeeper discovery provider (like all ECF discovery providers) should not change any of the client/publisher-provided service properties (or even assume a certain type for any of the service properties). As well, the services specified by the serviceTypeID (inside of IServiceInfo...provided to AdvertisedService constructor by client/publisher) should also not be changed or assumed to have any specific value. In this case, what's happening is that RSA is setting the services (inside the serviceTypeID) to: { osgirsvc } To be specific, it's doing it in this method: org.eclipse.ecf.osgi.services.remoteserviceadmin.ServiceInfoFactory.createServiceTypeID(EndpointDescription, IDiscoveryAdvertiser) on line 199 The other discovery API providers pass this value through untouched...and the EndpointDescriptionLocator code you reference > > ----- EndpointDesciptionLocater, line 807 ----- > private boolean matchServiceID(IServiceID serviceId) { > if (Arrays.asList(serviceId.getServiceTypeID().getServices()) > .contains(RemoteConstants.SERVICE_TYPE)) > return true; > return false; > } returns true...as long as getServices() returns { osgirsvcs } as it should. All I'm saying is: the zookeeper provider should have the same behavior as the other discovery providers...i.e. to pass through (without changing), the values of all the parts of the IServiceInfo specified in registerService(IServiceInfo)...including the services associated with the IServiceTypeID, and the discovery properties...including objectClass (if it appears in the discovery properties...as it does with RSA). How the service type id and service properties are represented internally to zookeeper is up to the zookeeper provider...but the values specified by the client in the IServiceInfo should not be assumed to be one type or another, and shouldn't be changed...this is so they have the same values when received by the receiver...i.e. so that the serviceTypeID.getServices() returns { osgirsvc }. I will rewrite DiscoverdService without any changes on properties. Internal handling in zoodiscvery is hard to understand for me at the moment. Even with some fixes: "osgisrvc" set and no changes with the properties there is this Exception. In the ServiceInfo object the property "objectClass" is set(!). But this is an String[], why is this decoded and handled as List in "AbstractMetadataFactorydecodeOSGiProperties(IServiceProperties props, Map osgiProperties)"? ----- [log;+0100 2011.02.13 20:41:36:259;ERROR;org.eclipse.ecf.osgi.services.remoteserviceadmin;org.eclipse.core.runtime.Status[plugin=org.eclipse.ecf.osgi.services.remoteserviceadmin;code=4;message=org.eclipse.ecf.osgi.services.remoteserviceadmin.DiscoveredEndpointDescriptionFactory:createDiscoveredEndpointDescription:Exception creating discovered endpoint description;severity4;exception=java.lang.IllegalArgumentException: objectClass is empty;children=[]]] java.lang.IllegalArgumentException: objectClass is empty at org.osgi.service.remoteserviceadmin.EndpointDescription.verifyObjectClassProperty(EndpointDescription.java:257) at org.osgi.service.remoteserviceadmin.EndpointDescription.<init>(EndpointDescription.java:110) at org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescription.<init>(EndpointDescription.java:83) at org.eclipse.ecf.osgi.services.remoteserviceadmin.AbstractMetadataFactory.decodeEndpointDescription(AbstractMetadataFactory.java:210) at org.eclipse.ecf.osgi.services.remoteserviceadmin.DiscoveredEndpointDescriptionFactory.createEndpointDescription(DiscoveredEndpointDescriptionFactory.java:104) at org.eclipse.ecf.osgi.services.remoteserviceadmin.DiscoveredEndpointDescriptionFactory.createDiscoveredEndpointDescription(DiscoveredEndpointDescriptionFactory.java:65) at org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator$LocatorServiceListener.getDiscoveredEndpointDescription(EndpointDescriptionLocator.java:893) at org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator$LocatorServiceListener.handleOSGiServiceEndpoint(EndpointDescriptionLocator.java:824) at org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator$LocatorServiceListener.handleService(EndpointDescriptionLocator.java:817) at org.eclipse.ecf.osgi.services.remoteserviceadmin.EndpointDescriptionLocator$LocatorServiceListener.serviceDiscovered(EndpointDescriptionLocator.java:800) at org.eclipse.ecf.provider.zookeeper.core.internal.Localizer.localize(Localizer.java:125) at org.eclipse.ecf.provider.zookeeper.node.internal.NodeReader.processResult(NodeReader.java:107) at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:527) Okay, it's just a String with: String org.eclipse.ecf.osgi.services.remoteserviceadmin.AbstractMetadataFactory.LIST_SEPARATOR = " " Okay, bug fixed.
But Scott, what type would you like to have in the objectClass property? I get a String[] from ZooKeeper, as all of the other properties. In AbstractMetadataFactory it's handled as a String with a AbstractMetadataFactory.LIST_SEPARATOR (" ") delimiter.
AbstractMetadataFactory.LIST_SEPARATOR in AbstractMetadataFactory is not visible, so is this delimiter safe for the future?
We could handle String[] AND String with delimiter as option in AbstractMetadataFactory too?
(In reply to comment #30) > Okay, bug fixed. > > But Scott, what type would you like to have in the objectClass property? I get > a String[] from ZooKeeper, as all of the other properties. In > AbstractMetadataFactory it's handled as a String with a > AbstractMetadataFactory.LIST_SEPARATOR (" ") delimiter. > > AbstractMetadataFactory.LIST_SEPARATOR in AbstractMetadataFactory is not > visible, so is this delimiter safe for the future? > > We could handle String[] AND String with delimiter as option in > AbstractMetadataFactory too? Hi Martin. In answer to your question...the RSA current implementation assumes a String value from the objectClass discovery property...which it then parses (in the metadata factory) to a String[]. But my general point is simply that no matter what the client puts in for objectClass, the receiver should get the same type and value out for that property. The ECF discovery API properties should *always* be the same values/value types for the receiver as for the sender...no matter what the name of the property. So, for example, if the sender/publisher sets this discovery property: discoveryProperties.setProperty("objectClass",new Long(3)); then on discovery the receiver should get Long val = (Long) discoveryProperties.getProperty("objectClass"); Although the abstract metadata factory could be modified to handle certain properties specially (e.g. objectClass), I don't think this makes very much sense. Rather, all discovery providers should just satisfy the ECF discovery API contract...and that's that discovery properties set by the client (including ones named objectClass) are *not modified* and *not assumed to have some certain type* (like String[])). Thanks for the help Martin...I am in direct touch with Wim (the committer responsible for the zookeeper provider) about this bug, and one way or another it will be addressed before the ECF 3.5/RSA release. You're right, I don't know about the other side. This is a possible fix/workaround (can do txt git diff in eclipse/at the moment):
public DiscoverdService(String path, Properties propMap) {
Assert.isNotNull(propMap);
this.uuid = path.split(INode._URI_)[0];
this.location = URI.create((String) propMap.remove(IService.LOCATION));
super.priority = Integer.parseInt((String) propMap
.remove(IService.PRIORITY));
super.weight = Integer.parseInt((String) propMap
.remove(IService.WEIGHT));
----- (+) -----
String[] services = (String[])propMap.remove(INode.NODE_PROPERTY_SERVICES);
propMap.put(Constants.OBJECTCLASS,
arrayToString((String[])propMap.remove(Constants.OBJECTCLASS), " "));
----- (-) -----
String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS);
if (services == null) {
services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES);
}
-----
String na = (String) propMap.remove(INode.NODE_PROPERTY_NAME_NA);
String[] protocols = (String[]) propMap
.remove(INode.NODE_PROPERTY_NAME_PROTOCOLS);
String[] scopes = (String[]) propMap
.remove(INode.NODE_PROPERTY_NAME_SCOPE);
super.properties = createServiceProperties(propMap);
this.serviceTypeID = ServiceIDFactory.getDefault().createServiceTypeID(
ZooDiscoveryContainer.getSingleton().getConnectNamespace(),
services, scopes, protocols, na);
super.serviceID = new ZooDiscoveryServiceID(ZooDiscoveryContainer
.getSingleton().getConnectNamespace(), this, serviceTypeID,
this.location);
super.serviceName = propMap.getProperty("component.name", location.toASCIIString());
}
----- (+) -----
public String arrayToString(String[] array, String delimiter) {
StringBuilder arTostr = new StringBuilder();
if (array.length > 0) {
arTostr.append(array[0]);
for (int i=1; i<array.length; i++) {
arTostr.append(delimiter);
arTostr.append(array[i]);
}
}
return arTostr.toString();
}
-----
private ServiceProperties createServiceProperties(Properties props) {
ServiceProperties result = new ServiceProperties();
for (Object k : props.keySet()) {
----- (+) -----
Object value = props.get(k);
----- (-) -----
Object value = (String) props.get(k);
-----
if (((String) k).startsWith(INode._BYTES_)) {
result.setPropertyBytes(((String) k).split(INode._BYTES_)[1],
(value + "").getBytes());
continue;
}
result.setProperty((String) k, value);
}
this.props = result.asProperties();
return result;
}
Fixed and released to master. Scott please try your RSA with the new ZooDiscovery. (In reply to comment #33) > Fixed and released to master. Scott please try your RSA with the new > ZooDiscovery. Hi Ahmed, bug (https://bugs.eclipse.org/bugs/show_bug.cgi?id=337345) could be connected to this bug and be caused by zoodiscovery. It comes up in EndpointDescriptionAdvertiser while service publication with zoodiscovery: protected IStatus doDiscovery(EndpointDescription endpointDescription, boolean advertise) { Assert.isNotNull(endpointDescription); String messagePrefix = advertise ? "Advertise" : "Unadvertise"; //$NON-NLS-1$ //$NON-NLS-2$ List<IStatus> statuses = new ArrayList<IStatus>(); // First get serviceInfoFactory IServiceInfoFactory serviceInfoFactory = getServiceInfoFactory(); if (serviceInfoFactory == null) return createErrorStatus(messagePrefix + " endpointDescription=" //$NON-NLS-1$ + endpointDescription + ". No IServiceInfoFactory is available. Cannot unpublish endpointDescription=" //$NON-NLS-1$ + endpointDescription); IDiscoveryAdvertiser[] discoveryAdvertisers = getDiscoveryAdvertisers(); if (discoveryAdvertisers == null || discoveryAdvertisers.length == 0) return createErrorStatus(messagePrefix + " endpointDescription=" //$NON-NLS-1$ + endpointDescription + ". No endpointDescriptionLocator advertisers available. Cannot unpublish endpointDescription=" //$NON-NLS-1$ + endpointDescription); for (int i = 0; i < discoveryAdvertisers.length; i++) { IServiceInfo serviceInfo = (advertise ? serviceInfoFactory .createServiceInfo(discoveryAdvertisers[i], endpointDescription) : serviceInfoFactory .removeServiceInfo(discoveryAdvertisers[i], endpointDescription)); if (serviceInfo == null) { statuses.add(createErrorStatus(messagePrefix + " endpointDescription=" //$NON-NLS-1$ + endpointDescription + ". Service Info is null. Cannot publish endpointDescription=" //$NON-NLS-1$ + endpointDescription)); continue; } // Now actually unregister with advertiser statuses.add(doDiscovery(discoveryAdvertisers[i], serviceInfo, advertise)); } return createResultStatus(statuses, messagePrefix + " endpointDesription=" + endpointDescription //$NON-NLS-1$ + ". Problem in unadvertise"); //$NON-NLS-1$ } Tested master with RSA and ZooDiscovery. For me this bug is fixed. I intend to test this fix with the ECF hello world examples today (Fri). Thanks to all for patience. (In reply to comment #36) > I intend to test this fix with the ECF hello world examples today (Fri). > Thanks to all for patience. First, the fix for this bug now works fine with the hello world examples...using the new ECF RSA admin. That's wonderful. Thanks Ahmed and Wim for addressing these issues in the zookeeper provider. I'm resolving this bug as fixed. To work with the 3.3.1 version of the Zookeeper bundle (i.e. org.apache.hadoop.zookeeper), I've modified the remote services hello examples (host and consumer) to use this bundle for the product...as the bundle id changed from org.apache.zookeeper to org.apache.hadoop.zookeeper. These product config changes have been released to master: http://git.eclipse.org/c/ecf/org.eclipse.ecf.git/commit/?id=f508cdb23ddbad09a8424665abd2103d521cb0a2 Again, thanks to all for a coordinated and rapid response to this bug. It will make consuming both the zookeeper provider and the ECF RSA impl (which of course can/does use the zookeeper provider) much easier for ECF 3.5 and beyond. |
For the remote service admin, one of the endpoint description properties has 'objectClass' as the key. This is as per the OSGi chap 122 specification. In testing the RSA implementation with ZooDiscovery, I discovered that when an EndpointDescription is discovered, that the ZooDiscovery implementation has this code in DiscoverdService.java<init> public DiscoverdService(String path, Properties propMap) { Assert.isNotNull(propMap); this.uuid = path.split(INode._URI_)[0]; this.location = URI.create((String) propMap.remove(IService.LOCATION)); super.priority = Integer.parseInt((String) propMap .remove(IService.PRIORITY)); super.weight = Integer.parseInt((String) propMap .remove(IService.WEIGHT)); String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS); if (services == null) { services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); } String na = (String) propMap.remove(INode.NODE_PROPERTY_NAME_NA); String[] protocols = (String[]) propMap .remove(INode.NODE_PROPERTY_NAME_PROTOCOLS); String[] scopes = (String[]) propMap .remove(INode.NODE_PROPERTY_NAME_SCOPE); super.properties = createServiceProperties(propMap); ... } When objectClass is one of the keys in propMap, it seems that this line of code: services = (String[]) propMap.remove(INode.NODE_PROPERTY_SERVICES); is *not* executed, and then when createServiceProperties(propMap) is executed (with INode.NODE_PROPERTY_SERVICES still present in the propMap, the following exception is thrown: 2011-01-25 21:30:10,727 - ERROR [pool-1-thread-3-EventThread:ClientCnxn$EventThread@579] - Caught unexpected throwable java.lang.ClassCastException: [Ljava.lang.String; cannot be cast to java.lang.String at org.eclipse.ecf.provider.zookeeper.core.DiscoverdService.createServiceProperties(DiscoverdService.java:70) at org.eclipse.ecf.provider.zookeeper.core.DiscoverdService.<init>(DiscoverdService.java:57) at org.eclipse.ecf.provider.zookeeper.node.internal.NodeReader.processResult(NodeReader.java:100) at org.apache.zookeeper.ClientCnxn$EventThread.run(ClientCnxn.java:527) It seems likely to me that 1) The objectClass key should *not* be removed from the service properties as it is with String[] services = (String[]) propMap.remove(Constants.OBJECTCLASS); 1a) The objectClass property in the published service properties should not be changed/messed with on publication...as it is needed for publishing RSA enpdoint descriptions. 2) The INode.NODE_PROPERTY_SERVICES property *should* probably always be removed from the properties (whether objectClass is present or not) Unfortunately, it looks like ZooDiscovery uses objectClass internally. This should probably be changed so that objectClass can be used as an endpoint description property as per the RSA spec.