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

Eliminate direct use of the gRPC transport package #9934

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

mbrannock
Copy link
Contributor

gRPC has moved the transport package to an internal-only directory. This
eliminates direct use of the transport package in the stress test in
favor of the error code from gRPC that represents a connection problem.

https://godoc.org/google.golang.org/grpc/internal/transport is the new
location for the package, which says it's not intended to be imported
directly. Instead, the maintainers suggested to use the code Unavailable
to detect a connection problem.

This change slightly reorganizes the stresser test error handling.

@@ -178,6 +142,64 @@ func (s *keyStresser) run() {
}
}

func isRetryableError(err) {
Copy link
Contributor

@jpbetz jpbetz Jul 18, 2018

Choose a reason for hiding this comment

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

should be func isRetryableError(err error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

case etcdserver.ErrStopped.Error():
// one of the etcd nodes stopped from failure injection
return true
case transport.ErrConnClosing.Desc:
Copy link
Contributor

Choose a reason for hiding this comment

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

will this compile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now.

@xiang90
Copy link
Contributor

xiang90 commented Jul 18, 2018

@mbrannock

I vaguely remember that we use the transport pkg error since the code is not unavailable when it is returned.

@gyuho
Copy link
Contributor

gyuho commented Jul 19, 2018

@xiang90 And at some point, clientv3 switched over to status package for error parsing. So, maybe we do not even need transport package errors anymore.

@mbrannock Can you address @xiang90 and @jpbetz's feedback? Then should be good to merge.

@mbrannock
Copy link
Contributor Author

Thanks everyone. Sorry about the first commit not being 100% correct. I figured out how to get my local client to build and run the tests, so I don't have to wait for travis to find problems... :-)

Please take a look and let me know if there is anything else I should change.

@xiang90 The gRPC dev told me that this particular transport error is translated to Unavailable. If it's coming back at something else, that might be a bug they'd be interested in.

@xiang90
Copy link
Contributor

xiang90 commented Jul 19, 2018

@mbrannock OK. Sounds good. LGTM

gRPC has moved the transport package to an internal-only directory. This
eliminates direct use of the transport package in the stress test in
favor of the error code from gRPC that represents a connection problem.

https://godoc.org/google.golang.org/grpc/internal/transport is the new
location for the package, which says it's not intended to be imported
directly. Instead, the maintainers suggested to use the code Unavailable
to detect a connection problem.

This change slightly reorganizes the stresser test error handling.
@mbrannock
Copy link
Contributor Author

It looks like the failing integration test may be a flake, for it's passing on my workstation. Is there a way to rerun in Travis?

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

Reran the CIs and confirmed that they all passed. Thanks for contribution!

@gyuho gyuho merged commit e9f710e into etcd-io:master Jul 19, 2018
@hexfusion hexfusion mentioned this pull request Aug 6, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants