Add a command line option to turn off retry attempts to resolve host

Add a command line option ("host-resolver-retry-attempts")
to specify the number of retry attempts to resolve host.


BUG=82580
TEST=host resolver unit tests
R=eroman
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85852

[ramant (doing other things), 2011-05-13 19:50:55]

Hi Eric and Jim,
  Added the command line option to turn off retry attempts for host resolution.
Would appreciate your comments.

thanks,
raman

[eroman, 2011-05-13 23:31:50]

Can you avoid using globals for this?

See for instance how the --host-resolver-parallelism flag is implemented (search
for kHostResolverParallelism).

We try to avoid globals since it makes it harder to configure things differently
for different dependencies --- in the multi-profile chrome world this is even
more important since different profiles might want different choices for this
configurations.

Also, please update the --host-resolver-parallelism flag to be in-line with the
attempts. (It's wording is no longer  correct).

Lastly, I suggest exposing the command line as a numeric property rather than a
boolean --> perhaps the flag controls the maximum number of retry attempts.

WDTY?

[jar (doing other things), 2011-05-14 03:11:58]

On 2011/05/13 23:31:50, eroman wrote:
> Can you avoid using globals for this?
> 
> See for instance how the --host-resolver-parallelism flag is implemented
(search
> for kHostResolverParallelism).
> 
> We try to avoid globals since it makes it harder to configure things
differently
> for different dependencies --- in the multi-profile chrome world this is even
> more important since different profiles might want different choices for this
> configurations.
> 
> Also, please update the --host-resolver-parallelism flag to be in-line with
the
> attempts. (It's wording is no longer  correct).
> 
> Lastly, I suggest exposing the command line as a numeric property rather than
a
> boolean --> perhaps the flag controls the maximum number of retry attempts.
> 
> WDTY?

I wonder if it is reasonable to expect that a user would want to vary this by
profile.  IMO, it is somewhat akin to the TCP retry attempts, or the underlying
DNS retry, which take place in the OS.  Such parameters are not configurable on
a per profile basis.

Or am I missing something?

I do like the use of a command line flag to change the default number of
retries.  ... but as I said... suspect this should be a global flag, and not a
per profile setting.

[ramant (doing other things), 2011-05-16 17:57:01]

Hi Eric and Jim,
  Made the changes Eric had suggested. Passed the number of retries as a command
line argument. Changed the code to be similar to kHostResolverParallelism
command line argument.

thanks much,
raman

[eroman, 2011-05-16 23:26:08]

lgtm after considering comment:
http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver_impl....

http://codereview.chromium.org/7011044/diff/8028/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7011044/diff/8028/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:605: // (not counting potential duplicate
attempts).
please elaborate this more, mention that the --host-resolver-retry-attempts must
be set to zero for this to be exact.

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver.h#new...
net/base/host_resolver.h:137: static const size_t kDefaultRetryAttempts = 4;
(optional) I recommend using a similar model to above, where the generic
constant is just 0, and each implementation can choose its own concrete default
value.

(For other implementations of HostResolver retry attempts might not make sense,
or work differently)

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver_impl....
net/base/host_resolver_impl.cc:552: if (unresponsive_delay >=
resolver_->maximum_unresponsive_delay())
can we get rid of maximum_unresponsive_delay now?

seems like it is redundant with max_retry_attempts, we should just have 1
constraint and have the other be inferred.

[ramant (doing other things), 2011-05-17 18:46:25]

Hi Eric,
  Made all the changes you had suggested. Could you take a look at this change
one more time?

thanks much,
raman

http://codereview.chromium.org/7011044/diff/8028/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7011044/diff/8028/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:605: // (not counting potential duplicate
attempts).
On 2011/05/16 23:26:08, eroman wrote:
> please elaborate this more, mention that the --host-resolver-retry-attempts
must
> be set to zero for this to be exact.

Done.

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver.h
File net/base/host_resolver.h (right):

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver.h#new...
net/base/host_resolver.h:137: static const size_t kDefaultRetryAttempts = 4;
On 2011/05/16 23:26:08, eroman wrote:
> (optional) I recommend using a similar model to above, where the generic
> constant is just 0, and each implementation can choose its own concrete
default
> value.
> 
> (For other implementations of HostResolver retry attempts might not make
sense,
> or work differently)

Done.

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/7011044/diff/8028/net/base/host_resolver_impl....
net/base/host_resolver_impl.cc:552: if (unresponsive_delay >=
resolver_->maximum_unresponsive_delay())
On 2011/05/16 23:26:08, eroman wrote:
> can we get rid of maximum_unresponsive_delay now?
> 
> seems like it is redundant with max_retry_attempts, we should just have 1
> constraint and have the other be inferred.

Done.

[eroman, 2011-05-17 22:53:26]

http://codereview.chromium.org/7011044/diff/16002/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/7011044/diff/16002/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:89: // resolution. Pass
HostResolver::kDefaultRetryAttempts to choose a default
Note that this does not work as described.

[ramant (doing other things), 2011-05-17 23:27:24]

HostResolverImpl constructor sets the maximum retry attempts to 4 if callers
pass kDefaultRetryAttempts.

Thanks Eric,
raman

http://codereview.chromium.org/7011044/diff/16002/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/7011044/diff/16002/net/base/host_resolver_impl...
net/base/host_resolver_impl.h:89: // resolution. Pass
HostResolver::kDefaultRetryAttempts to choose a default
On 2011/05/17 22:53:26, eroman wrote:
> Note that this does not work as described.

Fixed.

Done.

[eroman, 2011-05-18 23:08:32]

LGTM