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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years ago by bradfitz
Modified:
4 years ago
Reviewers:
rsc, iant
CC:
golang-dev, iant, cespare, rsc, dfc, 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/
4 years ago (2013-06-06 13:36:08 UTC) #1
iant
LGTM In CL description, s/coallesce/coalesce/.
4 years 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) ?
4 years 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 ...
4 years 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, ...
4 years ago (2013-06-10 18:24:49 UTC) #5
dfc
I like this, singleflight is very nice. I have a concern that the singleflight cache ...
4 years ago (2013-06-11 11:19:12 UTC) #6
dfc
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 ...
4 years 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 ...
4 years 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)) ...
4 years 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.
4 years 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
4 years 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 ...
4 years ago (2013-06-13 22:47:00 UTC) #12
iant
LGTM
4 years 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 ...
4 years ago (2013-06-14 13:21:20 UTC) #14
bradfitz
4 years 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 80a51fa-tainted