Bug 195392

Summary: initializeSubSystem() default impl should not set the port to 0
Product: [Tools] Target Management Reporter: Martin Oberhuber <mober.at+eclipse>
Component: RSEAssignee: Martin Oberhuber <mober.at+eclipse>
Status: RESOLVED FIXED QA Contact: Martin Oberhuber <mober.at+eclipse>
Severity: enhancement    
Priority: P3 Flags: dmcknigh: review+
Version: 2.0   
Target Milestone: 2.0.1   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
Patch removing the setPort(0) call none

Description Martin Oberhuber CLA 2007-07-04 10:55:10 EDT 
SubSystemConfiguration.initializeSubSystem() default implementation currently sets the port of its associated ConnectorService to 0.

The problem with this is, that the ConnectorService should know better than any associated SubSystem what a proper default value for its port property is. A second problem is that if any overrider wants to change the functionality and have a different default port value, the ConnectorService is marked dirty unnecessarily, because its port gets set to 0 and then back to the expected user value.

I assume that the functionality of setting the port to 0 during initialization comes from the days where RSE did not have a separate ConnectorService. But now, since there is one, the only proper place for specifying a default of the ConnectorService's port is in the ConnectorService's constructor. Subsystems who want to do it differently can override initializeSubSystem() in their own implementation.
Comment 1 Martin Oberhuber CLA 2007-07-04 11:00:13 EDT 
All current implementations of ConnectorService set the port to 0 in their constructor, except FTP (sets it to 21 -- the default) and daytime (sets it to 13).

I thus expect the following change in functionality for FTP and Daytime only:
  * maybe improved performance sine ConnectorService is no longer marked dirty
  * Subsystem Properties of newly created connections for Daytime and FTP will
    show the actual port being used rather than "0 (First-available)".
    I consider this an improvement since it better shows users where the port
    can be actually changed.

Note that existing subsystems which were created before this change will still have port "0" shown for Daytime and FTP since this port number (meaning "default port") is stored persistently.
Comment 2 Martin Oberhuber CLA 2007-07-04 11:04:52 EDT 
Created attachment 73033 [details]
Patch removing the setPort(0) call
Comment 3 Martin Oberhuber CLA 2007-07-04 11:08:33 EDT 
Dave could you please review whether you'd consider attached patch appropriate for 2.0.1? It does slightly change the functionality that had been documented in the APIDocs, but IMHO that functionality was not appropriate and removing it just improves the overall situation.
Comment 4 David McKnight CLA 2007-07-04 12:41:08 EDT 
This seems okay to me.
Comment 5 Martin Oberhuber CLA 2007-07-04 12:49:11 EDT 
Patch committed - thanks.