Thanks Sanjin for the patch. It looks pretty good. One tiny thing I'd like to point out is that it would be much nicer if a user can configure the selector timeout parameter. For example, we could leave the default '1000', and let people reconfigure that by calling the following statement:
connector.setMaxConnectTimeoutCheckInterval(50); // ms I'm not sure if this property name is good, but it's my idea that we need to make it configurable rather than hard-coding it. In the same context, we could make the idle time check interval also configurable. WDYT? 2008-02-06 (수), 10:09 -0800, Sangjin Lee 쓰시길: > I have filed a JIRA issue (https://issues.apache.org/jira/browse/DIRMINA-527), > and submitted a patch for this. > > In the patch I added IoConnector.setConnectTimeoutMillis(), and deprecated > getConnectTimeout()/setConnectTimeout() in favor of the new ones. Perhaps > one could go as far as removing them? I wasn't sure... > > Also, the select timeout is now tied to the connect timeout, but still is no > longer than 1 second. > > I also imposed a somewhat arbitrary minimum legal connect timeout value of > 50 ms, but again we could discuss what the right value should be if we need > one. > > What do you think? > > Thanks, > Sangjin > > > On Feb 5, 2008 7:46 AM, Sangjin Lee <[EMAIL PROTECTED]> wrote: > > > Right, that's why I said the connect timeout limitation seems tied to the > > select timeout... > > Hard-coded 1 second select timeout will interfere with sub-second connect > > timeout value. One obvious suggestion is to set the select timeout to be > > the same as the connect timeout. That way, we're pretty sure that the > > connect timeout will be honored. > > > > One small drawback is that we'd end up with a busy select loop if the > > connect timeout is too small. This could be prevented by having a minimum > > connect/select timeout value... > > > > Also, note that the 1-second timeout is pervasive elsewhere (like > > processor, etc.). Not sure if we need to change them also. Maybe not right > > now... > > > > If you guys don't mind, I'll file a bug (both for 1.1.x and 2.x) and > > submit a patch also... How's that sound? > > > > BTW, what is up with the 1 second sleep in the try-catch clause in the > > same code area? We can leave it as is? > > > > } catch (Throwable e) { > > > > ExceptionMonitor.getInstance().exceptionCaught(e); > > > > > > try { > > > > Thread.sleep(1000); > > > > } catch (InterruptedException e1) { > > > > ExceptionMonitor.getInstance > > ().exceptionCaught(e1); > > > > } > > > > } > > > > Thanks, > > Sangjin > > > > > > On Feb 5, 2008 7:29 AM, Julien Vermillard <[EMAIL PROTECTED]> wrote: > > > > > On Tue, 05 Feb 2008 13:55:15 +0100 > > > Niklas Therning <[EMAIL PROTECTED]> wrote: > > > > You would also need to make sure that the current IoConnector > > > > implementations can support millisecond timeouts. I think that would > > > > mean that AbstractPollingIoConnector.Worker needs to be changed. > > > > Specifically the line > > > > > > > > boolean selected = select(1000); > > > > > > > > To support milliseconds we could simply change this into > > > > > > > > boolean selected = select(1); > > > > > > > > but that would be very bad for performance. > > > > > > > > Instead, one could use an adaptive timeout in the call to select() > > > > which depends on the current ConnectRequests' timeouts. Or totally > > > > redesign things to use a ScheduledExecutorService or similar instead. > > > > > > > > > > Yes Niklas is right, I forgot the impact on worker loop. > > > > > > At first look an adaptive select timeout or a redesign using some kind > > > of ExecutorService doesn't look like a trivial move.. > > > > > > Whats the lowest connect timeout needed if it's under 1 seconds ? > > > > > > Julien > > > > > > > -- what we call human nature is actually human habit -- http://gleamynode.net/
signature.asc
Description: This is a digitally signed message part
