Community
Participate
Working Groups
Only the DayTime example uses ServiceSubSystem. The service subsystems (Shell, File, Processes) do not. Likewise IServiceSubSystem is really only used for type testing at run time. It would make more sense to eliminate the concept of a service subsystem as a Java type and make it a runtime aspect of subsystems and their configurations. As part of the minor refactoring in bug 197036 to support subsystem configuration switching in a more general way SubSystem now implements IServiceSubSystem anyway so some of this refactoring is already done.
This sounds like a breaking API change. Is it worth it?
I believe this change does make sense, but after looking a bit closer I think we can pare it down to just getting rid of the abstract classes ServiceSubSystem and ServiceSubSystemConfiguration. ServiceSubSystem is not used. ServiceSubSystemConfiguration is only used by DaytimeSubSystemConfiguration. I would keep IServiceSubSystem and its interface hierarchy. So the only modification to the classes in teh base would be to DaytimeSubSystemConfiguration. I've already made compatible changes to SubSystem to have it implement IServiceSubSystem. While this change is not binary compatible it is source compatible if you don't use ServiceSubSystemConfiguration or ServiceSubSystem. ServiceSubSystemConfiguration has only one method that is different than SubSystemConfiguration (supportsNestedFilters), all the other methods are duplicates of those in SubSystemConfiguration. All of the methods in ServiceSubSystem are duplicates of SubSystem and ServiceSubSystem is not referenced anywhere in our code. The reason that is isn't referenced is that service subsystems must subclass under a different hierarchy for implementation - the subsystem resource related hierarchy (i.e. RemoteFileSubSystem). Java doesn't have multiple inheritance so no one subclasses ServiceSubSystem.
Ok, good points - you have a go from me. This change will make the subsystem hierarchy simpler, thus easier to understand.
I guess the one thing I'm wondering is this: We have some client code that works differently depending on whether a given IRemoteFileSubSystem, for instance, is actually a) an IServiceSubSystem (such that we could get the IFileService), or it is b) not an IServiceSubSystem but "legacy" and implements all the file service operations inside itself. Your change sounds like that distinction would go away, and thus the client code would not work any more? See ... instanceof IFileServiceSubSystem in RSEFileStoreImpl for instance.
I checked all references of IServiceSubSystem. There is one place that broke by having SubSystem implement it. A patch for this break will be attached. Please review. Other than this it looks to me that only the DayTime example and the documentation need to be updated in order to get rid of ServiceSubSytem and ServiceSubSystemConfiguration.
Created attachment 89061 [details] patch for SystemViewSubSystemAdapter.getAbsoluteName(Object) If SubSystem implements IServiceSubSystem then getServiceType() will sometimes return null and will corrupt the absolute name. This fixes that problem.
Created attachment 89062 [details] mylyn/context/zip
Migration guide notes: The ServiceSubSystem and ServiceSubSystemConfiguration classes have been removed and have been replaced with run time tests. A subsystem is deemed a service subsystem if it returns a non-null response to getServiceType(). Be aware that IServiceSubSystem is now implemented by the SubSystem class. Code that was testing for IServiceSubSystem should likely test for getServiceType() being non-null. Any subsystem implementation that subclassed ServiceSubSystem should now subclass SubSystem. Any subsystem configuration that subclassed ServiceSubSystemConfiguration should now subclass SubSystemConfiguration. Any service subsystem - must implement getServiceType() and return the interface class of its service type. For example SSHFileSubSystem returns IFileService.class. - must implement canSwitchTo(SubSystemConfiguration) if it is capable of switching configurations - must implement internalSwitchServiceSubSystemConfiguration(SubSystemConfiguration) if canSwitchTo(SubSystemConfiguration) is true.
Created attachment 89088 [details] complete patch for removing ServiceSubSystem and ServiceSubSystemConfiguration This removes the package org.eclipse.rse.core.servicesubsystem entirely. Manifests have been updated in this patch.
Here is another API change: * IServiceSubSystem.getServiceType() was not documented previously, but before the change one could expect that it cannot return null. * After the change, IServiceSubSystem.getServiceType() can return null and must be documented as such. Client code that needs to work differently for handling subsystems or service subsystems now must look like this: if ( (ss instanceof IServiceSubSystem) && (IServiceSubSystem)ss.getServiceType() ! null ) { //handle the service case... } whereas before the change, the "instanceof" check was sufficient. Note, though, that for specific kinds of services, "instanceof" still works: if (ss instanceof IFileServiceSubSystem) { //... } because IFileServiceSubSystem.getFileService() is defined to not return null (well API Docs do not specify this explicitly, but it is Eclipse common use that ability to return null must be documented explicitly). I'm wondering whether we should get rid of IServiceSubSystem and IServiceSubSystemConfiguration as well. For extenders of ISubSystem it should not make a difference when the extra two methods are added to ISubSystem: API Docs for ISubSystem / ISubSystemConfiguration say that "...extenders should not implement these interfaces directly but extend (SubSystem / SubSystemConfiguration) instead". So they get IServiceSubSystemConfiguration for free anyways and the change is backward compatible in this respect. Removing the extra two interfaces would make client code simpler: if (ss.getServiceType() != null) { //handle the service case... } My personal take is that with the current change we break extenders of Service Subsystems already, as soon as they reference ServiceSubSystem somewhere. so we should make the change complete and remove the interfaces as well. That way, those clients which make the "...instanceof IServiceSubSystem" check get a compile error and are forced to make the necessary "...!=null" check that's necessary with the current change already. Comments? Does anybody think we need separate IServiceSubSystem* interfaces?
I need to look at this. I suspect you are correct. This would make the idea of service subsystems strictly dependent on the non-null return of getServiceType(). Having the test of this nature allows a subsystem to be refactored to contain a service layer after initial deployment without breaking its API. However, getting rid of these interfaces is a bit more work. There are quite a few type tests of IServiceSubSystem and IServiceSubSystemConfiguration. I think these could all be replaced by tests for non-null getServiceType() since getServiceType() is supported by IServiceSubSystemConfiguration and would be pulled up into ISubSystemConfiguration. ServiceSubSystemConfiguration would have to implement the "non-service" version of these methods to be overridden by service subsystem configurations. I think DaveM needs to comment on this issue. It seems to me that all subsystem configurations in the same category should return the same service type. That is, the category is the plugin.xml equivalent of the service type. Am I right here?
(In reply to comment #11) > I think DaveM needs to comment on this issue. > It seems to me that all subsystem configurations in the same category should > return the same service type. That is, the category is the plugin.xml > equivalent of the service type. Am I right here? Yes, I think all subsystem configurations in the same category should return the same service type. I wonder whether something else is needed though since some configurations can have more than one service type. For example, some file service configurations (i.e. dstore) have a search service as well as a file service.
(In reply to comment #11) > However, getting rid of these interfaces is a bit more work. There are quite a > few type tests of IServiceSubSystem and IServiceSubSystemConfiguration. I think But that's exactly my point. If there is a substantial amount of non-trivial refactorings due to removing IServiceSubSystem*, it is likely that the other change (which modified the behavior of IServiceSubSystem*) would cause failures without these refactorings. > ServiceSubSystemConfiguration would have to implement the "non-service" > version of these methods to be overridden by service subsystem configurations. I believe you are talking about the SubSystemConfiguration abstract base class, since ServiceSubSystemConfiguration was removed already? > It seems to me that all subsystem configurations in the same category should > return the same service type. That is, the category is the plugin.xml > equivalent of the service type. Am I right here? Not sure, but my naive answer would be that in the "files" category one could also have a subsystem returning files that does not expose the Service APIs (i.e. return null for getServiceType() and just implement IRemoteFileSubSystem. Also, since the service is kind of an implementation detail from a subsystem point of view, yet another files subsystem could use a totally different kind of service. In practice, I could imagine such "same category, different service" cases for Processes. Processes are very different to retrieve on different OS's, yet the kind of resources could be similar thus the category could be the same.
(In reply to comment #13) > But that's exactly my point. If there is a substantial amount of non-trivial > refactorings due to removing IServiceSubSystem*, it is likely that the other > change (which modified the behavior of IServiceSubSystem*) would cause failures > without these refactorings. OK. You've convinced me. :) > I believe you are talking about the SubSystemConfiguration abstract base class, > since ServiceSubSystemConfiguration was removed already? Correct. > Not sure, but my naive answer would be that in the "files" category one could > also have a subsystem returning files that does not expose the Service APIs > (i.e. return null for getServiceType() and just implement IRemoteFileSubSystem. > Also, since the service is kind of an implementation detail from a subsystem > point of view, yet another files subsystem could use a totally different kind > of service. > > In practice, I could imagine such "same category, different service" cases for > Processes. Processes are very different to retrieve on different OS's, yet the > kind of resources could be similar thus the category could be the same. Also a good point. I withdraw my concern. I will submit a patch for review by you and DaveM.
Created attachment 89209 [details] Removing (I)ServiceSubSystem* This patch removes all 4 types and makes the necessary changes to test for getServiceType() instead of instanceof.
Created attachment 89210 [details] mylyn/context/zip
Excellent, diligent work as always Dave. I really like this change since it makes the code quite a bit simpler. Thanks also for your changes to Javadoc which explain the situation. Just one request: When making this change, can you please make at least the interfaces (ISystemRegistry, ISubSystem, ISubSystemConfiguration) compile without warnings [admittedly some of these warnings come from myself making the warning level for Javadoc a little more stringent]. I'll leave it up to you whether you want to add a line of comment like this on each changed file: * David Dykstal (IBM) - [217556] Get rid of IServiceSubSystem In general, I prefer keeping these "file comments" to one line. If DaveM is also fine, please go forward committing soon since it's a sizeable change.
Migration Guide: The following types have been removed: IServiceSubSystemConfiguration, IServiceSubSystem, ServiceSubSystemConfiguration, and ServiceSubSystem. Their function has been pulled up into ISubSystemConfiguration, ISubSystem, SubSystemConfiguration, and SubSystem. If you previously subclassed the classes ServiceSubSystem or ServiceSubSystemConfiguration you should now subclass SubSystem or SubSystemConfiguration respectively. If you previously implemented IServiceSubSystem or IServiceSubSystemConfiguration you should now implement ISubSystem or ISubSystemConfiguration respectively. Note that you already inherit this implementation if you are subclassing SubSystem or SubSystemConfiguration. If you made an instanceof test for any of these types you can replace that test with getServiceType() != null. Service subsystems and service subsystem configurations must now return the interface type to that call. If you previously used getServiceType() on either a subsystem or a subsystem configuration you must be prepared to handle the case where this may return null. A null value indicates that the subject subsystem or subsystem configuration is not a service subsystem or service subsystem configuration.
API Change Summary: The types IServiceSubSystem, IServiceSubSystemConfiguration, ServiceSubSystem, and ServiceSubSystemConfiguration have been removed. There function has been pulled up into ISubSystem, ISubSystemConfiguration, SubSystem, and SubSystemConfiguration respectively. SubSystem now contain defaults implementations for the following methods. If you are implementing a subsystem layer that exposes an underlying service you will need to override these. getServiceType() SubSystemConfiguration now contains default implementations for the following methods. If you are implementing a subsystem layer that exposes an underlying service you will need to override these. setConnectorService(IHost host, IConnectorService connectorService); getServiceType(); getServiceImplType(); getService(IHost host); For both subsystems and subsystem configurations the getServiceType() method has changed. It will now return null if the subsystem or subsystem configuration is not associated with a service. It will return the interface type of the service if it is associated with a service. This is the preferred method of testing whether or not a subsystem (configuration) supports a service layer.
Committing changes. Resolving.
