river-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Trasuk <tras...@stratuscom.com>
Subject Re: LookupDiscovery.ResponseListener configuration
Date Fri, 12 Oct 2012 14:22:58 GMT

On Fri, 2012-10-12 at 08:48, Simon IJskes - QCG wrote:
> On 12-10-12 14:19, Greg Trasuk wrote:
> > I just had a quick look at the code for LookupDiscovery.  Hard to tell
> > without going deeper, but it looks like it sends multicast requests on a
> > set of network interfaces configured by the 'multicastInterfaces' config
> > entry, and includes a host name configured by the 'multicastRequestHost'
> > configuration entry.  The ServerSocket listed above will listen for
> > responses to the multicast (which are unicast) on all the network
> > interfaces.  So I don't think the 'new ServerSocket(0)' is wrong.  It
> > might be slightly more correct to create a listener for each interface,
> > but I suspect the ServerSocket(0) is a reasonable hack.
> But you cannot predict on which port it will end. So you cannot 
> preconfigure a firewall for it.

OK, now I get it.  So we need a config entry for
'multicastDiscoveryPort' to go along with 'multicastDiscoveryHost'.  Or
perhaps we should allow the host to be of the form 'host:port' and parse
it out.

> > So, I think that LookupDiscovery is already configurable enough.
> > Similarly, the real solution for any other uses of
> > 'InetAddress.getLocalHost()' in the codebase is to make the component
> > properly configurable with a host name.
> >
> > Now, as for your thought on a configurable strategy for figuring out the
> > preferred local hostname, there's already a utility class called
> > 'com.sun.jini.config.ConfigUtil' that has methods for 'getHostName()'
> > and 'getHostAddress()'.  Currently they just call
> > 'InetAddress.getLocalHost()'.  Perhaps we could add a pluggable strategy
> > there.  Probably just have another version of the methods take a
> > strategy object.  That way it would be usable from inside a
> > Configuration object.
> Agree on making the lookup accesible from the config class. But i think 
> it is not wide enough. (I have 71 text matches). For instance there is a 
> call in reggie for the 'unicastDiscoveryHost'. All the serverendpoints 
> call it. LookupDiscovery. If we replace all the calls to a single method 
> in river, we have a central point where we can set/check/determine the 
> 'river host'. We can do a Logger.warning there if localhost.
> When you want to deploy on a host with some broken localhost/ own 
> hostname idea, and no way to fix this, you cannot deploy there, even 
> with the configuration. I see no harm in adding an extra central 
> intercept point. And it shouldn't have to change the current behaviour.

I see what you mean.  I'm torn here between "what's right" and "what's
convenient for developers".  I think in general, any time we use a host
name, it should be configurable from a Configuration object, so having a
utility class that provides a sensible local host object (and port, I
guess) is the "right" way to do it.  At the same time, I realize that
not everyone likes the Configuration mechanism, and even if they do,
having to configure something as basic as the local host name is a
nuisance.  So I can see your point about having a reasonable default.

Perhaps we should try to do both.  Find any current usages (what text
were you looking up to get 73 hits?) and make sure the components have
the correct configuration options.  Then use a central utility class as
the default rather than 'getLocalHost()' if no config is provided.

Part of me also wonders if this isn't what the socket factory mechanism
(ServerSocket.setSocketFactory()' is supposed to handle. Although I must
confess I really don't like that mechanism, because it isn't specific
about the scope of the socket factory (classloader, application, what?).



> -- 
> QCG, Software voor het MKB, 071-5890970, http://www.qcg.nl
> Quality Consultancy Group b.v., Leiderdorp, Kvk Den Haag: 28088397

View raw message