Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move exponential backoff to DNS resolver from resolver.ClientConn #4270

Merged
merged 58 commits into from Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from 57 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
dcab165
Save progress
zasweq Mar 15, 2021
8754401
Save progress
zasweq Mar 16, 2021
6bb242d
Clean up
zasweq Mar 16, 2021
8299dad
Clean up
zasweq Mar 16, 2021
da102eb
vet
zasweq Mar 16, 2021
fbda038
vet
zasweq Mar 16, 2021
0ebd685
Added logic to tests
zasweq Mar 16, 2021
7cb488e
Added unit test for exponential backoff
zasweq Mar 16, 2021
ba01815
Vet
zasweq Mar 16, 2021
15ec449
Added another unit test
zasweq Mar 16, 2021
f0a6aed
Changed test
zasweq Mar 16, 2021
72f5538
Fixed failing tests
zasweq Mar 16, 2021
3bc67ff
Vet
zasweq Mar 16, 2021
b31f41f
Deleted old tests
zasweq Mar 16, 2021
8bc20c1
Vet
zasweq Mar 16, 2021
d20f77a
Fixed data race
zasweq Mar 16, 2021
9fe80c1
Vet
zasweq Mar 16, 2021
6b1401c
Fixed timeout
zasweq Mar 16, 2021
797bf3e
Fixed race
zasweq Mar 16, 2021
e5fa7fc
Fixed race
zasweq Mar 16, 2021
a0a24b5
Rewrote failing unit test
zasweq Mar 16, 2021
f39b7b2
Vet
zasweq Mar 16, 2021
fb44fe1
Vet
zasweq Mar 16, 2021
0721699
Fixed race conditions in tests
zasweq Mar 16, 2021
5574d9a
Changed time
zasweq Mar 16, 2021
8572acb
Responded to Doug's comments
zasweq Mar 18, 2021
54d1fd2
Responded to Doug's comments
zasweq Mar 18, 2021
33695df
vet
zasweq Mar 18, 2021
100ba1b
vet
zasweq Mar 18, 2021
8646061
vet
zasweq Mar 18, 2021
e75a12b
Vet
zasweq Mar 18, 2021
f54ecb4
Vet
zasweq Mar 18, 2021
2ecb953
Vet
zasweq Mar 18, 2021
513a51d
Deleted callback in tests
zasweq Mar 19, 2021
c61426f
Added polling to wait group
zasweq Mar 19, 2021
c3d8c01
Responded to Doug's comments
zasweq Mar 26, 2021
32801ec
Vet
zasweq Mar 26, 2021
090069d
Cleaned up comments
zasweq Mar 26, 2021
ac3e18e
Responded to Doug's comments
zasweq Mar 26, 2021
653a564
Responded to Doug's comments
zasweq Mar 26, 2021
f51aa71
Vet
zasweq Mar 26, 2021
625a9e2
Save progress before cleanup
zasweq Apr 1, 2021
e384bab
Clean up
zasweq Apr 1, 2021
d8ffa44
Vet
zasweq Apr 1, 2021
e923d61
Vet
zasweq Apr 1, 2021
0960018
More cleanup
zasweq Apr 1, 2021
98ed270
Save Progress
zasweq Apr 5, 2021
e8f3fff
Changed logic
zasweq Apr 5, 2021
96f2bc0
Incorporated Doug's idea
zasweq Apr 7, 2021
e325361
Responded to Doug's comments
zasweq Apr 12, 2021
e90b3a4
Responded to Doug's comments
zasweq Apr 12, 2021
d9e7620
Responded to Doug's comments
zasweq Apr 13, 2021
fc31662
Vet
zasweq Apr 13, 2021
7c26bb3
Responded to Doug's comments
zasweq Apr 14, 2021
e40bbaa
Vet
zasweq Apr 14, 2021
40b52ef
Responded to Doug's comments
zasweq Apr 20, 2021
6d02b03
Vet
zasweq Apr 20, 2021
e9eed86
Switched send to blocking
zasweq Apr 20, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
90 changes: 0 additions & 90 deletions balancer_conn_wrappers_test.go

This file was deleted.

17 changes: 1 addition & 16 deletions dialoptions.go
Expand Up @@ -66,11 +66,7 @@ type dialOptions struct {
minConnectTimeout func() time.Duration
defaultServiceConfig *ServiceConfig // defaultServiceConfig is parsed from defaultServiceConfigRawJSON.
defaultServiceConfigRawJSON *string
// This is used by ccResolverWrapper to backoff between successive calls to
// resolver.ResolveNow(). The user will have no need to configure this, but
// we need to be able to configure this in tests.
resolveNowBackoff func(int) time.Duration
resolvers []resolver.Builder
resolvers []resolver.Builder
}

// DialOption configures how we set up the connection.
Expand Down Expand Up @@ -596,7 +592,6 @@ func defaultDialOptions() dialOptions {
ReadBufferSize: defaultReadBufSize,
UseProxy: true,
},
resolveNowBackoff: internalbackoff.DefaultExponential.Backoff,
}
}

Expand All @@ -611,16 +606,6 @@ func withMinConnectDeadline(f func() time.Duration) DialOption {
})
}

// withResolveNowBackoff specifies the function that clientconn uses to backoff
// between successive calls to resolver.ResolveNow().
//
// For testing purpose only.
func withResolveNowBackoff(f func(int) time.Duration) DialOption {
return newFuncDialOption(func(o *dialOptions) {
o.resolveNowBackoff = f
})
}

// WithResolvers allows a list of resolver implementations to be registered
// locally with the ClientConn without needing to be globally registered via
// resolver.Register. They will be matched against the scheme used for the
Expand Down
39 changes: 26 additions & 13 deletions internal/resolver/dns/dns_resolver.go
Expand Up @@ -34,6 +34,7 @@ import (

grpclbstate "google.golang.org/grpc/balancer/grpclb/state"
"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal/backoff"
"google.golang.org/grpc/internal/envconfig"
"google.golang.org/grpc/internal/grpcrand"
"google.golang.org/grpc/resolver"
Expand All @@ -46,6 +47,9 @@ var EnableSRVLookups = false

var logger = grpclog.Component("dns")

// A global to stub out in tests.
var newTimer = time.NewTimer

func init() {
resolver.Register(NewBuilder())
}
Expand Down Expand Up @@ -143,7 +147,6 @@ func (b *dnsBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts

d.wg.Add(1)
go d.watcher()
d.ResolveNow(resolver.ResolveNowOptions{})
return d, nil
}

Expand Down Expand Up @@ -201,28 +204,38 @@ func (d *dnsResolver) Close() {

func (d *dnsResolver) watcher() {
defer d.wg.Done()
backoffIndex := 1
for {
select {
case <-d.ctx.Done():
return
case <-d.rn:
}

state, err := d.lookup()
if err != nil {
// Report error to the underlying grpc.ClientConn.
d.cc.ReportError(err)
} else {
d.cc.UpdateState(*state)
err = d.cc.UpdateState(*state)
}

// Sleep to prevent excessive re-resolutions. Incoming resolution requests
// will be queued in d.rn.
t := time.NewTimer(minDNSResRate)
var timer *time.Timer
if err == nil {
// Success resolving, wait for the next ResolveNow. However, also wait 30 seconds at the very least
// to prevent constantly re-resolving.
backoffIndex = 1
timer = time.NewTimer(minDNSResRate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should use newTimer, too, or else we can't test all the timing aspects that we should be testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newTimer is extremely coupled to like 15 tests. The success path (with MinDNSResRate) is already tested within the unit tests. Ex: https://github.com/zasweq/grpc-go/blob/move-backoff-to-dns-resolver/internal/resolver/dns/dns_resolver_test.go#L1046.

select {
case <-d.ctx.Done():
timer.Stop()
return
case <-d.rn:
}
} else {
// Poll on an error found in DNS Resolver or an error received from ClientConn.
timer = newTimer(backoff.DefaultExponential.Backoff(backoffIndex))
backoffIndex++
}
select {
case <-t.C:
case <-d.ctx.Done():
t.Stop()
timer.Stop()
return
case <-timer.C:
}
}
}
Expand Down