Could someone take a look at this (DIRMINA-527) and address it? Thanks! Regards, Sangjin
On Feb 12, 2008 9:21 PM, 이희승 (Trustin Lee) <[EMAIL PROTECTED]> wrote: > We could let the connector automatically adjust the select timeout value > if the current select timeout is greater than the specified connect > timeout. > > 2008-02-12 (화), 21:16 -0800, Sangjin Lee 쓰시길: > > Yes, it would be great if users can configure the select timeout. > > One small issue that I have, however, is if the select timeout is longer > > than the connect timeout. Then it is possible that the connect timeout > will > > not always be honored because select will not wake up until select > timeout > > pops. > > > > It is understood that it would happen only if there are no other connect > > events happening, but a select timeout that is longer than the connect > > timeout will effectively interfere with the proper connect timeout. How > > shall we address that situation? > > > > Thanks, > > Sangjin > > > > > > On Feb 12, 2008 8:43 PM, 이희승 (Trustin Lee) <[EMAIL PROTECTED]> wrote: > > > > > 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/ > > > > -- > what we call human nature is actually human habit > -- > http://gleamynode.net/ >
