Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1326)

Issue 10079043: code review 10079043: net: coalesce duplicate in-flight DNS lookups (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by bradfitz
Modified:
11 years, 5 months ago
Reviewers:
rsc, iant
CC:
golang-dev, iant, cespare, rsc, dave_cheney.net, rog, remyoudompheng
Visibility:
Public.

Description

net: coalesce duplicate in-flight DNS lookups In Issue 5625, Russ says: "We should at least have a cache of inflight lookups, so that 100 simultaneous dials of one host name don't do the work 100x. That's easy and (assume we forget the answer once they all get it) doesn't pose any consistency problems. It just merges simultaneous work." This brings in singleflight (unexported) from Google / Camlistore, but without its tests. Maybe we should put it somewhere in the standard library. But not now. Update issue 5625

Patch Set 1 #

Patch Set 2 : diff -r ca145611fa96 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r ca145611fa96 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 4 : diff -r 330478300ab0 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 5 : diff -r 52d53d0e177e https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -3 lines) Patch
M src/pkg/net/lookup.go View 1 2 3 2 chunks +24 lines, -3 lines 0 comments Download
A src/pkg/net/singleflight.go View 1 2 3 4 1 chunk +53 lines, -0 lines 0 comments Download

Messages

Total messages: 15
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 6 months ago (2013-06-06 13:36:08 UTC) #1
iant
LGTM In CL description, s/coallesce/coalesce/.
11 years, 6 months ago (2013-06-06 22:42:40 UTC) #2
cespare
https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go File src/pkg/net/lookup.go (right): https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go#newcode16 src/pkg/net/lookup.go:16: return addrs, err return lookupHost(host) ?
11 years, 6 months ago (2013-06-07 08:17:59 UTC) #3
rsc
I agree with Ian: please return separate copies of the value if there were multiple ...
11 years, 5 months ago (2013-06-10 17:47:00 UTC) #4
bradfitz
I believe that was Dominik's position, but I agree. Will do. On Mon, Jun 10, ...
11 years, 5 months ago (2013-06-10 18:24:49 UTC) #5
dave_cheney.net
I like this, singleflight is very nice. I have a concern that the singleflight cache ...
11 years, 5 months ago (2013-06-11 11:19:12 UTC) #6
dave_cheney.net
https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go File src/pkg/net/lookup.go (right): https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/lookup.go#newcode12 src/pkg/net/lookup.go:12: lookupHostMerge doesn't mean much to me, how about cachedLookupHost ...
11 years, 5 months ago (2013-06-11 11:19:32 UTC) #7
bradfitz
On Tue, Jun 11, 2013 at 1:19 PM, <dave@cheney.net> wrote: > I like this, singleflight ...
11 years, 5 months ago (2013-06-11 11:36:15 UTC) #8
rog
https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/singleflight.go File src/pkg/net/singleflight.go (right): https://codereview.appspot.com/10079043/diff/5001/src/pkg/net/singleflight.go#newcode27 src/pkg/net/singleflight.go:27: func (g *singleflight) Do(key string, fn func() (interface{}, error)) ...
11 years, 5 months ago (2013-06-11 11:40:49 UTC) #9
bradfitz
Hello golang-dev@googlegroups.com, iant@golang.org, cespare@gmail.com, rsc@golang.org, dave@cheney.net, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 5 months ago (2013-06-13 22:30:16 UTC) #10
cespare
https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go File src/pkg/net/singleflight.go (right): https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go#newcode14 src/pkg/net/singleflight.go:14: dups int gofmt
11 years, 5 months ago (2013-06-13 22:39:15 UTC) #11
remyoudompheng
https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go File src/pkg/net/singleflight.go (right): https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go#newcode49 src/pkg/net/singleflight.go:49: if c, ok := g.m[key]; ok { I don't ...
11 years, 5 months ago (2013-06-13 22:47:00 UTC) #12
iant
LGTM
11 years, 5 months ago (2013-06-13 22:47:09 UTC) #13
rsc
LGTM with fix https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go File src/pkg/net/singleflight.go (right): https://codereview.appspot.com/10079043/diff/16001/src/pkg/net/singleflight.go#newcode49 src/pkg/net/singleflight.go:49: if c, ok := g.m[key]; ok ...
11 years, 5 months ago (2013-06-14 13:21:20 UTC) #14
bradfitz
11 years, 5 months ago (2013-06-14 15:59:50 UTC) #15
*** Submitted as https://code.google.com/p/go/source/detail?r=e255db359405 ***

net: coalesce duplicate in-flight DNS lookups

In Issue 5625, Russ says: "We should at least have a cache of
inflight lookups, so that 100 simultaneous dials of one host
name don't do the work 100x. That's easy and (assume we forget
the answer once they all get it) doesn't pose any consistency
problems. It just merges simultaneous work."

This brings in singleflight (unexported) from Google /
Camlistore, but without its tests. Maybe we should put it
somewhere in the standard library. But not now.

Update issue 5625

R=golang-dev, iant, cespare, rsc, dave, rogpeppe, remyoudompheng
CC=golang-dev
https://codereview.appspot.com/10079043
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b