Thanks for the tips, I've made these changes locally. I have a further question though - should we preserve the public method signatures in ServerAnnotationProcessor that take a "startPort" parameter? This parameter is only specified by the FrameworkRunner class, which always just passes through a value of "1".
>From my POV this parameter is now redundant as we start the transports on either the port defined in the annotation, or else a random port (or the same for multiple transports, if not specified). Colm. On Mon, Dec 1, 2014 at 5:39 PM, Emmanuel Lécharny <[email protected]> wrote: > Le 01/12/14 18:10, Colm O hEigeartaigh a écrit : > >> That could be a good addition to the annotation though. Not terribly > > useful except for protocol using TCP and UDP (like kerberos...) > > > > Ok, I will take a look and see if I can contribute a patch for this. > > > > One further query on this issue: I've noticed that the ports generated > for > > the LDAP server tend to be fairly random, whereas the ports generated for > > the KDC server always tend to start at 1024. > > The code that process the transports is the following : > > private static void createTransports( LdapServer ldapServer, > CreateTransport[] transportBuilders ) > { > if ( transportBuilders.length != 0 ) > { > for ( CreateTransport transportBuilder : transportBuilders ) > { > String protocol = transportBuilder.protocol(); > int port = transportBuilder.port(); > ... > > if ( port <= 0 ) > { > try > { > ServerSocket ss = new ServerSocket( 0 ); > > port = ss.getLocalPort(); > > ss.close(); > > That is for both the KdcServer and the LdapServer. > > However, if you don't stipulate a Transport, the KdcServer will try to > pick teh first available port with this code : > > private static KdcServer createKdcServer( CreateKdcServer > createKdcServer, DirectoryService directoryService, > int startPort ) > { > ... > CreateTransport[] transportBuilders = createKdcServer.transports(); > > if ( transportBuilders == null ) > { > // create only UDP transport if none specified > UdpTransport defaultTransport = new UdpTransport( > AvailablePortFinder.getNextAvailable( startPort ) ); > kdcServer.addTransports( defaultTransport ); > } > else if ( transportBuilders.length > 0 ) > { > for ( CreateTransport transportBuilder : transportBuilders ) > { > Transport t = createTransport( transportBuilder, > startPort ); > startPort = t.getPort() + 1; > kdcServer.addTransports( t ); > } > } > > As you can see, we use AvailablePortFinder.getNextAvailable( startPort ) > in the case no Transport is defined, and in such a case, it will start > picking a port at 1024. > > This is clearly abcient code, and we should use new ServerSocket( 0 ). > There are other parts of the ServerAnnotationProcessor that should be > reviewed and fixed. > > The createKdcServer could also be modified to make both TCP and UDP > having teh same port if the transports are both provided in the > annotations : > > for ( CreateTransport transportBuilder : transportBuilders ) > { > Transport t = createTransport( transportBuilder, > startPort ); > startPort = t.getPort() + 1; > kdcServer.addTransports( t ); > } > > In this loop, we can affect the same port to the second Transport when > the first Transport startPort is <= 0. For instance, adding a third > parameter to the createTransport() method - the requested port - could > be used. If it's > 0, then we use this port. The method would then be : > > public static Transport createTransport( CreateTransport > transportBuilder, int startPort, int requestedPort ) > > where startPort will simply be ignored when requestedPort >= 0. > > > -- Colm O hEigeartaigh Talend Community Coder http://coders.talend.com
