-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
e2e: etcdctl test for proxy no-sync #4330
Conversation
8197ae7
to
a4660e7
Compare
/cc @heyitsanthony @xiang90 Please review. This only test #3894 but I can add more |
} | ||
defer func() { | ||
if errC := epc.Close(); errC != nil { | ||
if !strings.Contains(errC.Error(), "os: process already finished") { |
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.
this is not needed. Will remove.
@@ -305,3 +330,91 @@ func spawnWithExpect(args []string, expected string) error { | |||
} | |||
return proc.Close() | |||
} | |||
|
|||
func TestBasicOpsV2CtlWatchWithProxy(t *testing.T) { |
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.
move the etcdctl stuff to etcdctl_test.go
?
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.
Sure.
@heyitsanthony Just moved to |
}() | ||
|
||
endpoint, be := "", "" | ||
for _, p := range epc.procs { |
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.
probably worth extending etcdProcessCluster
so it's possible to do this instead of the confusing loop thing:
if proxies := epc.Proxies(); len(proxies) != 0 {
endpoint = proxies[0].cfg.acurl.String()
} else if backends := epc.Backends(); len(backends) != 0 {
endpoint = backends[0].cfg.acurl.String()
}
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.
ok good idea. Will try that. Thanks
lgtm aside from comments |
4ca02a7
to
c700ff7
Compare
@heyitsanthony Just updated. Thanks! |
@@ -289,6 +313,27 @@ func (epc *etcdProcessCluster) Close() (err error) { | |||
return err | |||
} | |||
|
|||
// proxies returns only the proxy etcdProcess. | |||
func (epc *etcdProcessCluster) proxies() []*etcdProcess { |
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.
return epc.procs[epc.cfg.clusterSize:]
return spawnWithExpect(putArgs, value) | ||
} | ||
|
||
func etcdctlWatch(epc *etcdProcessCluster, key, value string, noSync bool, done chan struct{}, errChan chan 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.
this only needs errChan
since donec <- struct{}{}
can be replaced with errChan <- nil
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.
up to you if you think it's worth doing this one
Just updated with leaky goroutine checks added. |
lgtm |
Thanks. Will merge after green lights. |
e2e: etcdctl test for proxy no-sync
For #3894.