-
Notifications
You must be signed in to change notification settings - Fork 304
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
Extract BackendPool interface into three 3 separate interfaces #424
Conversation
3f800c2
to
be5ea3e
Compare
/assign @freehan |
12b9cf3
to
cca40ed
Compare
78a9e0d
to
adeb109
Compare
@@ -0,0 +1,180 @@ | |||
package backends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copyright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed everywhere.
0291d72
to
18d5abc
Compare
All tests are now migrated over to new interfaces. Added a new test file called integration_test.go which contains some tests carried over from the original. Those are failing so need to figure out why. |
18d5abc
to
7e56d86
Compare
Update: All tests pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor stuff
pkg/backends/backends.go
Outdated
// will dictate whether we search for an existing legacy health check. | ||
hcLink, err := b.ensureHealthCheck(sp, hasLegacyHC) | ||
if err != nil { | ||
// Implements Pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update implements Pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/backends/backends.go
Outdated
} | ||
|
||
// Delete deletes the Backend for the given port. | ||
// Implements Pool. | ||
func (b *Backends) Delete(name string) (err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete implements ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use non-implicit err
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to get rid of it because of the defer.
pkg/backends/backends.go
Outdated
if !oldBackends.Equal(newBackends) { | ||
backendService.Backends = targetBackends | ||
return b.cloud.UpdateBetaGlobalBackendService(backendService) | ||
// Implements Pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetLocalSnapshot implements Pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/backends/backends.go
Outdated
func (b *Backends) edgeHop(be *composite.BackendService, igLinks []string) error { | ||
addIGs, err := getInstanceGroupsToAdd(be, igLinks) | ||
// Implements Pool. | ||
func (b *Backends) Get(name string, version meta.Version) (*composite.BackendService, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get implements ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/backends/backends.go
Outdated
func (b *Backends) Status(name string) string { | ||
backend, err := b.cloud.GetGlobalBackendService(name) | ||
if err != nil || len(backend.Backends) == 0 { | ||
// Implements Pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Health implements ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/backends/ig_linker.go
Outdated
const maxRPS = 1 | ||
|
||
// InstanceGroupLinker handles linking backends to InstanceGroup's. | ||
type InstanceGroupLinker struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have to be exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it for this and others as well.
pkg/backends/ig_linker.go
Outdated
} | ||
} | ||
|
||
// Implements Link. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix all comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/backends/ig_linker.go
Outdated
} | ||
} | ||
|
||
// We first try to create the backend with balancingMode=RATE. If this + return addIGs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird formatting on this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/backends/integration_test.go
Outdated
@@ -0,0 +1,210 @@ | |||
/* | |||
Copyright 2015 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
7e56d86
to
2d9cae0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
This PR extracts the BackendPool interface into 3 separate interfaces:
This allows us to break the current BackendPool implementation up into more manageable chunks and each chunk is much more testable. Furthermore, some implementations, namely BackendPool and Syncer, can be easily reused.
/assign @bowei