Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-527

should be able to set connect timeout in milliseconds

Details

    • Improvement
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 1.0.8, 1.1.5, 2.0.0-M1
    • 2.0.0-M2
    • Core
    • None

    Description

      Currently, IoConnector allows setting connect timeouts only in seconds. The minimum value of allowed connect timeout is 1 second. There are cases where one needs to have a shorter connect timeout than 1 second and will finer granularity than seconds.

      I suggest introducing the ability to set connect timeout in milliseconds and deprecating the getter/setter in seconds in favor of the ms getter/setter.

      See the discussion thread at http://www.nabble.com/connect-timeout-to15281787s16868.html.

      Attachments

        1. DIRMINA-527.patch
          6 kB
          Sangjin Lee

        Activity

          sjlee0 Sangjin Lee added a comment -

          A proposed patch (based on the trunk).

          • Introduces IoConnector.setConnectTimeoutMillis(), and deprecates getConnectTimeout()/setConnectTimeout().
          • Adjust the select timeout to be the smaller of the connectTimeout() or 1 second.

          Please review the changes, and accept them if you guys are OK with it. I'd also like to see the changes propagated to 1.1.x (and 1.0.x too?). Do we need separate JIRA issues for them?

          Thanks,
          Sangjin

          sjlee0 Sangjin Lee added a comment - A proposed patch (based on the trunk). Introduces IoConnector.setConnectTimeoutMillis(), and deprecates getConnectTimeout()/setConnectTimeout(). Adjust the select timeout to be the smaller of the connectTimeout() or 1 second. Please review the changes, and accept them if you guys are OK with it. I'd also like to see the changes propagated to 1.1.x (and 1.0.x too?). Do we need separate JIRA issues for them? Thanks, Sangjin

          The API for 1.0/1.1 has been frozen. Since this patch introduces changes in the API it should only go into trunk. Please, anyone, correct me if I'm wrong.

          BTW, the patch looks great! One tiny detail, I think the coding conventions we're using in MINA mandates spaces around operators. E.g. 60*1000L => 60 * 1000L. See http://mina.apache.org/developer-guide.html#DeveloperGuide-CodingConvention for more info.

          niklas@trillian.se Niklas Therning added a comment - The API for 1.0/1.1 has been frozen. Since this patch introduces changes in the API it should only go into trunk. Please, anyone, correct me if I'm wrong. BTW, the patch looks great! One tiny detail, I think the coding conventions we're using in MINA mandates spaces around operators. E.g. 60*1000L => 60 * 1000L. See http://mina.apache.org/developer-guide.html#DeveloperGuide-CodingConvention for more info.
          sjlee0 Sangjin Lee added a comment -

          Hmm... Is there any hope for changes in 1.1.x? It would be adding new methods, not removing existing ones, so backward compatibility is there...

          sjlee0 Sangjin Lee added a comment - Hmm... Is there any hope for changes in 1.1.x? It would be adding new methods, not removing existing ones, so backward compatibility is there...

          The ruling seems to be "no API changes whatsoever in 1.1.x". See https://issues.apache.org/jira/browse/DIRMINA-397?focusedCommentId=12517492#action_12517492 for a precedent

          greywulf Eero Nevalainen added a comment - The ruling seems to be "no API changes whatsoever in 1.1.x". See https://issues.apache.org/jira/browse/DIRMINA-397?focusedCommentId=12517492#action_12517492 for a precedent
          elihusmails Mark Webb added a comment -

          I have applied the patch associated with this issue. I will go ahead and mark this issue as fixed.

          elihusmails Mark Webb added a comment - I have applied the patch associated with this issue. I will go ahead and mark this issue as fixed.
          elihusmails Mark Webb added a comment -

          I applied the associated patch after reviewing the patch. This only adds to the precision of the timeout. The methods that dealt with getting and setting the connection timeout in seconds have been deprecated and a note in the javadoc says that the millisecond-based methods should be used.

          elihusmails Mark Webb added a comment - I applied the associated patch after reviewing the patch. This only adds to the precision of the timeout. The methods that dealt with getting and setting the connection timeout in seconds have been deprecated and a note in the javadoc says that the millisecond-based methods should be used.
          trustin Trustin Lee added a comment - We have a little bit more to do http://mina.markmail.org/search/?q=connect%20timeout#query:connect%20timeout+page:1+mid:wp3ewpnfew7msthj+state:results
          elihusmails Mark Webb added a comment -

          What is the purpose of the MINIMUM_CONNECT_TIMEOUT field? If it can be changed via a call to setConnectTimeoutMillis(long), then why have it? Trustin mentions in the TODO:

          "Make this configurable and automatically adjusted if the timeout a user specified is smaller than the current minimum connect timeout."

          So if the minimum connection timeout and connection timeout can both be configured, why not just have one field? The only way this makes sense to me is that if you try and set the connection timeout to be lower than the minimum we should throw an exception.

          elihusmails Mark Webb added a comment - What is the purpose of the MINIMUM_CONNECT_TIMEOUT field? If it can be changed via a call to setConnectTimeoutMillis(long), then why have it? Trustin mentions in the TODO: "Make this configurable and automatically adjusted if the timeout a user specified is smaller than the current minimum connect timeout." So if the minimum connection timeout and connection timeout can both be configured, why not just have one field? The only way this makes sense to me is that if you try and set the connection timeout to be lower than the minimum we should throw an exception.
          trustin Trustin Lee added a comment -

          For example, we could start from 1000ms selector timeout at the first time. If a user specifies a connect timeout value smaller than 1000ms, we can adjust the 1000ms to the connect timeout value specified (or its half). By doing so, we can minimize unnecessary CPU consumption, because most users will use the timeout value of 60 seconds or something similar to that. If a user wants a millisecond-level timeout (e.g. 9ms), then he will pay for what he or she wants, but it's not the case for most people.

          trustin Trustin Lee added a comment - For example, we could start from 1000ms selector timeout at the first time. If a user specifies a connect timeout value smaller than 1000ms, we can adjust the 1000ms to the connect timeout value specified (or its half). By doing so, we can minimize unnecessary CPU consumption, because most users will use the timeout value of 60 seconds or something similar to that. If a user wants a millisecond-level timeout (e.g. 9ms), then he will pay for what he or she wants, but it's not the case for most people.
          elihusmails Mark Webb added a comment -

          I have checked in an update that makes the minimum timeout field configurable.

          elihusmails Mark Webb added a comment - I have checked in an update that makes the minimum timeout field configurable.
          elihusmails Mark Webb added a comment -

          checked in an update that makes the minimum connection timeout field configurable

          elihusmails Mark Webb added a comment - checked in an update that makes the minimum connection timeout field configurable
          trustin Trustin Lee added a comment -

          Looks very good in its implementation!

          However, I think the property name 'minimumConnectTimeout' doesn't represent what it is actually. What do you think about renaming it to connectTimeoutCheckInterval or something similar? Do you have any idea?

          trustin Trustin Lee added a comment - Looks very good in its implementation! However, I think the property name 'minimumConnectTimeout' doesn't represent what it is actually. What do you think about renaming it to connectTimeoutCheckInterval or something similar? Do you have any idea?
          elihusmails Mark Webb added a comment -

          Nothing is really jumping out to me on this one so your idea is fine with me.

          elihusmails Mark Webb added a comment - Nothing is really jumping out to me on this one so your idea is fine with me.

          People

            elihusmails Mark Webb
            sjlee0 Sangjin Lee
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: