A/B experiment for re-establishing TCP connections

A/B experiment for re-establishing TCP connections.

We would like to test the impact of automatic retries when a TCP connection exceeds a certain threshold before it gets back an ACK. We are observing a fair number of sockets where the connection had been established, but the sockets
were not used thereafter.


r=mbelche



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57276

[ziadh, 2010-08-23 03:07:57]

Hi Mike,
Please let me know what you think :)

Regards,

[Mike Belshe, 2010-08-23 04:05:10]

I'd like to see naming unification between "BackupJobs" and "syn_retry".  Let me
know if you vehemently object.

Any chance we can do PLT measurements for this field trial as well?

http://codereview.chromium.org/3191019/diff/19001/20003
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3191019/diff/19001/20003#newcode436
chrome/common/chrome_switches.cc:436: 
nit: extra blank line here.

http://codereview.chromium.org/3191019/diff/19001/20006
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/3191019/diff/19001/20006#newcode503
net/socket/client_socket_pool_base.cc:503: void
ClientSocketPoolBaseHelper::set_retry_syn_enabled(bool enabled) {
I find the two different naming schemes for the same feature to be unfortunate.

The old name was "ConnectBackupJob", the new name is "retry_syn"....  Should we
just have one name for these?

[ziadh, 2010-08-23 11:32:19]

http://codereview.chromium.org/3191019/diff/19001/20003
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3191019/diff/19001/20003#newcode436
chrome/common/chrome_switches.cc:436: 
On 2010/08/23 04:05:10, Mike Belshe wrote:
> nit: extra blank line here.

Done.

http://codereview.chromium.org/3191019/diff/19001/20006
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/3191019/diff/19001/20006#newcode503
net/socket/client_socket_pool_base.cc:503: void
ClientSocketPoolBaseHelper::set_retry_syn_enabled(bool enabled) {
On 2010/08/23 04:05:10, Mike Belshe wrote:
> I find the two different naming schemes for the same feature to be
unfortunate.
> 
> The old name was "ConnectBackupJob", the new name is "retry_syn"....  Should
we
> just have one name for these?

Agreed, "ConnectBackupJob" it is :) ( or Jobs :) )

[ziadh, 2010-08-23 11:32:52]

I also added histograms for PLT.

[ziadh, 2010-08-24 18:40:49]

Ping! :-) Sorry if I'm asking you at a bad time.

[Mike Belshe, 2010-08-25 00:01:28]

lgtm

http://codereview.chromium.org/3191019/diff/68002/38006
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3191019/diff/68002/38006#newcode143
chrome/common/chrome_switches.cc:143: // for TCP sockets.
nit: the comment is out of sync with the new naming :-)

http://codereview.chromium.org/3191019/diff/68002/38009
File net/socket/client_socket.cc (right):

http://codereview.chromium.org/3191019/diff/68002/38009#newcode47
net/socket/client_socket.cc:47: FieldTrialList::Find("ConnnectBackupJobs") &&
style: spacing should be indented 4, I think...

http://codereview.chromium.org/3191019/diff/68002/38010
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/3191019/diff/68002/38010#newcode32
net/socket/client_socket_pool_base.cc:32: bool connect_backup_jobs_enabled =
true;
nit:  if you don't mind prefixing this with "g_", it makes it look more like a
global...

[ziadh, 2010-08-25 01:23:23]

http://codereview.chromium.org/3191019/diff/68002/38006
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3191019/diff/68002/38006#newcode143
chrome/common/chrome_switches.cc:143: // for TCP sockets.
On 2010/08/25 00:01:28, Mike Belshe wrote:
> nit: the comment is out of sync with the new naming :-)

Done.

http://codereview.chromium.org/3191019/diff/68002/38009
File net/socket/client_socket.cc (right):

http://codereview.chromium.org/3191019/diff/68002/38009#newcode47
net/socket/client_socket.cc:47: FieldTrialList::Find("ConnnectBackupJobs") &&
On 2010/08/25 00:01:28, Mike Belshe wrote:
> style: spacing should be indented 4, I think...

Yep, silly MSVC always undoing my spacing changes :)

http://codereview.chromium.org/3191019/diff/68002/38010
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/3191019/diff/68002/38010#newcode32
net/socket/client_socket_pool_base.cc:32: bool connect_backup_jobs_enabled =
true;
On 2010/08/25 00:01:28, Mike Belshe wrote:
> nit:  if you don't mind prefixing this with "g_", it makes it look more like a
> global...

Done.