-
Notifications
You must be signed in to change notification settings - Fork 64
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
Expose Connection<I>
#329
Merged
Merged
Expose Connection<I>
#329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's considered an anti-pattern to deny warnings in the source itself. It's better to configure this in the environment, which we already do for CI with `-D warnings`.
This is a small step towards removing the `ConnectionPool`. The `ConnectionPool` is used for a number of things, one of which is enabling the `Endpoint` APIs that operate exclusively on `SocketAddr`s, rather than connection handles. This commit exposes the `Connection<I>` type, and updates the `Endpoint::new`, `Endpoint::connect_to`, and `Endpoint::connect_to_any` methods to return `Connection<I>` rather than `SocketAddr`. For now, the only public method on `Connection<I>` is `remote_address`, which callers can then use with the existing (unchanged) `SocketAddr`-based `Endpoint` APIs. Future commits will introduce additional `Connection` methods to replace these `SocketAddr`-based APIs. BREAKING CHANGE: `Endpoint::new`, `Endpoint::connect_to`, and `Endpoint::connect_to_any` now use `Connection<I>` instead of `SocketAddr` in their return type.
This mostly cleans up existing information about connection pooling and priority. Connection pooling info has been added to each method, to make sure the behaviour is clear in each case.
The new version has tighter defaults, which should prevent us from committing dodgy commit types like `wip`. The `GITHUB_TOKEN` has also been removed, since the repository is public and the action is read only.
Rather than `Endpoint` having getters for `addr` (`get_socket_addr_by_id`) and `id` (`get_connection_id`) there are now only getters for `Connection`, either by `addr` (`get_connection_by_addr`) or by `id` (`get_connection_by_id`). For now this mostly adds indirection for callers, who can call `Connection::id` or `Connection::remote_address` on the result to achieve the same outcome as before. BREAKING CHANGE: `Endpoint::get_connection_id` and `Endpoint::get_socket_addr_by_id` have been removed. `Endpoint::get_connection_by_addr` and `Endpoint::get_connection_by_id` can be used instead to get a `Connection`, from which the `id` or `remote_address` can be retrieved.
`CARGO_HTTP_MULTIPLEXING` has been disabled, which seems to have resolved our network woes, and `-D warnings` is removed from `RUSTFLAGS` for that job, since new nightlies may introduce additional lints that we wouldn't otherwise block. The SHA of the action has been pinned also, to ensure there are no surprise updates.
The timing-based tests for this module are flaky, but since `ConnectionDeduplicator` isn't long for this world we'll live with bumping the timeout for now.
lionel-faber
approved these changes
Sep 21, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
366e0dc chore: don't deny warnings in source
It's considered an anti-pattern to deny warnings in the source itself.
It's better to configure this in the environment, which we already do
for CI with
-D warnings
.ce50e47 feat!: return
Connection
fromEndpoint::connect_*
This is a small step towards removing the
ConnectionPool
. TheConnectionPool
is used for a number of things, one of which isenabling the
Endpoint
APIs that operate exclusively onSocketAddr
s,rather than connection handles.
This commit exposes the
Connection<I>
type, and updates theEndpoint::new
,Endpoint::connect_to
, andEndpoint::connect_to_any
methods to return
Connection<I>
rather thanSocketAddr
. For now, theonly public method on
Connection<I>
isremote_address
, which callerscan then use with the existing (unchanged)
SocketAddr
-basedEndpoint
APIs. Future commits will introduce additional
Connection
methods toreplace these
SocketAddr
-based APIs.BREAKING CHANGE:
Endpoint::new
,Endpoint::connect_to
, andEndpoint::connect_to_any
now useConnection<I>
instead ofSocketAddr
in their return type.4f0cf52 docs: expand and clarify
Endpoint
method documentationThis mostly cleans up existing information about connection pooling and
priority. Connection pooling info has been added to each method, to make
sure the behaviour is clear in each case.
bc0b5c7 docs: fix a broken intra-doc link
7d0b710 ci: upgrade
wagoid/commitlint-github-action
The new version has tighter defaults, which should prevent us from
committing dodgy commit types like
wip
. TheGITHUB_TOKEN
has alsobeen removed, since the repository is public and the action is read
only.
54d1040 refactor!: replace
Endpoint
addr
/id
gettersRather than
Endpoint
having getters foraddr
(
get_socket_addr_by_id
) andid
(get_connection_id
) there are nowonly getters for
Connection
, either byaddr
(
get_connection_by_addr
) or byid
(get_connection_by_id
).For now this mostly adds indirection for callers, who can call
Connection::id
orConnection::remote_address
on the result toachieve the same outcome as before.
BREAKING CHANGE:
Endpoint::get_connection_id
andEndpoint::get_socket_addr_by_id
have been removed.Endpoint::get_connection_by_addr
andEndpoint::get_connection_by_id
can be used instead to get a
Connection
, from which theid
orremote_address
can be retrieved.b3bdf3c ci: fix
cargo-udeps
actionCARGO_HTTP_MULTIPLEXING
has been disabled, which seems to haveresolved our network woes, and
-D warnings
is removed fromRUSTFLAGS
for that job, since new nightlies may introduce additional lints that we
wouldn't otherwise block. The SHA of the action has been pinned also, to
ensure there are no surprise updates.
7110a3f test: bump timeouts in
ConnectionDeduplicator
testsThe timing-based tests for this module are flaky, but since
ConnectionDeduplicator
isn't long for this world we'll live withbumping the timeout for now.