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

transport: conn, abort OpenShardConn faster #286

Closed
wants to merge 1 commit into from

Conversation

Kulezi
Copy link
Contributor

@Kulezi Kulezi commented Sep 12, 2022

Currently OpenShardConn retries every time it encounters an error, regardless of whether it is context related or not, bloating the test log in case of TestContextsIntegration.

This PR changes the behavior of OpenShardConn, so it checks if context is done before trying to open a new connection, leaving immediately if it's done with appropriate error message.

Relates to #281, solving the context case, another PR will be made resolving the case of whether source port was busy or the error was something else.

@Kulezi Kulezi force-pushed the pp/open-shard-conn-ctx branch from 061f322 to e78bfd2 Compare September 12, 2022 10:41
@Kulezi Kulezi marked this pull request as ready for review September 12, 2022 10:44
Copy link

@piodul piodul left a comment

Choose a reason for hiding this comment

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

I think that something different should be done. In general, we should return every error, unless it's either EADDRINUSE or EPERM. For other errors it doesn't really make sense to retry - for example, in case of a timeout it does not make sense because a different port is not better than any other.

Something like that is already done in gocql, see here: https://github.com/scylladb/gocql/blob/e3e27db4440c6a1f0029322f2bb0e266589d9f0f/scylla.go#L631-L633

I included EPERM because it turns out that on Windows some programs with administrator privileges can reserve ports for themselves and Windows returns a WinApi equivalent of EPERM if you try to use it; we had an issue like that in the rust driver: scylladb/scylla-rust-driver#205

@Kulezi Kulezi changed the title transport: conn, abort OpenShardConn faster when context is done transport: conn, abort OpenShardConn faster Sep 23, 2022
@Kulezi
Copy link
Contributor Author

Kulezi commented Sep 23, 2022

I think that something different should be done. In general, we should return every error, unless it's either EADDRINUSE or EPERM. For other errors it doesn't really make sense to retry - for example, in case of a timeout it does not make sense because a different port is not better than any other.

Something like that is already done in gocql, see here: https://github.com/scylladb/gocql/blob/e3e27db4440c6a1f0029322f2bb0e266589d9f0f/scylla.go#L631-L633

I included EPERM because it turns out that on Windows some programs with administrator privileges can reserve ports for themselves and Windows returns a WinApi equivalent of EPERM if you try to use it; we had an issue like that in the rust driver: scylladb/scylla-rust-driver#205

Implemented that as part of #295 bugfix PR. Closing this one its favor.

@Kulezi Kulezi closed this Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants