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

Expose TCP parameters #1321

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Conversation

BenFradet
Copy link
Contributor

What

This PR exposes useful TCP parameters to the user through the configuration

Why

refer to issue #31

@spetz
Copy link
Contributor

spetz commented Nov 3, 2024

Hey Ben,
Thanks for the contribution! Looks good in general, I've added some comments here an there to make it work with the current conventions (especially given Option<T> in config structs). Also, please run the following to ensure that CI checks will pass:

  • cargo fmt
  • cargo machete
  • cargo sort --workspace
  • cargo clippy --all-targets --all-features -- -D warnings (unless you use MacOS, as there might be some TCP related warnings which do not exist on Linux).
  • cargo test or cargo nextest run

@BenFradet BenFradet mentioned this pull request Nov 3, 2024
@BenFradet BenFradet force-pushed the issue-31/expose-tcp-params branch from e9cf6dc to 2668fde Compare November 3, 2024 15:41
@hubcio
Copy link
Contributor

hubcio commented Nov 3, 2024

@BenFradet remove the hash and number from commit message title and preferably squash everything into one commit ;)

@BenFradet BenFradet changed the title Expose TCP parameters #31 Expose TCP parameters Nov 3, 2024
@BenFradet BenFradet force-pushed the issue-31/expose-tcp-params branch 3 times, most recently from a09d8b0 to 5ac5e8c Compare November 3, 2024 22:07
hubcio added a commit that referenced this pull request Nov 4, 2024
Some information was missing:
#1321 (comment)

Co-authored-by: Hubert Gruszecki <[email protected]>
@hubcio
Copy link
Contributor

hubcio commented Nov 4, 2024

There's test that needs fixing:

failures:

---- tcp::tcp_socket::tests::given_override_defaults_socket_should_be_configured stdout ----
thread 'tcp::tcp_socket::tests::given_override_defaults_socket_should_be_configured' panicked at server/src/tcp/tcp_socket.rs:65:9:
assertion `left == right` failed
  left: 851968
 right: 425984
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

https://github.com/iggy-rs/iggy/pull/1321/checks

@BenFradet BenFradet force-pushed the issue-31/expose-tcp-params branch from 1382ebc to 2483aa0 Compare November 4, 2024 18:06
@BenFradet
Copy link
Contributor Author

I moved the ipv6 setting up to the tcp config as it was making more sense to me

@coveralls
Copy link

coveralls commented Nov 4, 2024

Pull Request Test Coverage Report for Build 11672948482

Details

  • 81 of 101 (80.2%) changed or added relevant lines in 8 files are covered.
  • 5 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.02%) to 75.14%

Changes Missing Coverage Covered Lines Changed/Added Lines %
sdk/src/clients/consumer.rs 0 1 0.0%
server/src/tcp/tcp_listener.rs 10 11 90.91%
server/src/tcp/tcp_server.rs 2 3 66.67%
server/src/tcp/tcp_tls_listener.rs 0 17 0.0%
Files with Coverage Reduction New Missed Lines %
server/src/streaming/systems/messages.rs 1 69.7%
server/src/tcp/tcp_tls_listener.rs 1 0.0%
server/src/quic/listener.rs 1 83.33%
server/src/configs/config_provider.rs 1 85.05%
server/src/streaming/systems/users.rs 1 88.15%
Totals Coverage Status
Change from base Build 11663060116: 0.02%
Covered Lines: 23107
Relevant Lines: 30752

💛 - Coveralls

@spetz
Copy link
Contributor

spetz commented Nov 4, 2024

Looks good, thanks! Please bump the server version to 0.4.71 and SDK to 0.6.33 and we can merge this :)

spetz
spetz previously approved these changes Nov 4, 2024
@spetz spetz merged commit b9650bf into apache:master Nov 4, 2024
17 checks passed
@BenFradet BenFradet deleted the issue-31/expose-tcp-params branch November 5, 2024 09:13
spetz pushed a commit that referenced this pull request Nov 10, 2024
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.

4 participants