Skip to content

Conversation

@muzarski
Copy link
Contributor

@muzarski muzarski commented Apr 1, 2025

Fixes: #1027

This PR introduces configuration option for shard-aware local port range.

Since not all ranges are valid (e.g. empty one, or one including reserved ports i.e. ports < 1024), I introduced a simple wrapper called ShardAwarePortRange which validates provided range in constructor.

Currently, Sharder is pub, so are its fields, and its methods responsible for computing local ports. This is why, I couldn't modify the sharder in any of the following ways:

  • adding ShardAwarePortRange field to the Sharder
  • providing ShardAwarePortRange parameter to Sharder methods

This is why, I introduced separate set of methods which additionally accept the port range. Current public methods are using new methods by providing a default range (ephemeral port range [49152, 65535]).

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • [ ] I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@muzarski muzarski self-assigned this Apr 1, 2025
@muzarski muzarski added this to the 1.1.0 milestone Apr 1, 2025
@github-actions
Copy link

github-actions bot commented Apr 1, 2025

cargo semver-checks found no API-breaking changes in this PR.
Checked commit: f04784b

@muzarski muzarski force-pushed the shard-aware-port-range branch 2 times, most recently from abaf9ea to 21a0e2a Compare April 2, 2025 12:50
@muzarski muzarski requested review from Lorak-mmk and wprzytula April 2, 2025 12:52
wprzytula
wprzytula previously approved these changes Apr 2, 2025
Lorak-mmk
Lorak-mmk previously approved these changes Apr 2, 2025
muzarski added 3 commits April 2, 2025 16:54
Introduced ShardAwarePortRange (simple wrapper over RangeInclusive<u16> with validation)
and exposed configuration options for shard-aware local port range.
These needed to be introduce as a separate methods, because current
methods are `pub`. The public methods use new methods by providing
the default port range.

For now, I'm leaving new methods as `pub(crate)`. Reviewers can decide
whether we want to expose them to users.
@muzarski muzarski dismissed stale reviews from Lorak-mmk and wprzytula via f04784b April 2, 2025 14:55
@muzarski muzarski force-pushed the shard-aware-port-range branch from 21a0e2a to f04784b Compare April 2, 2025 14:55
@muzarski muzarski requested review from Lorak-mmk and wprzytula April 2, 2025 14:55
@Lorak-mmk Lorak-mmk merged commit e193fab into scylladb:main Apr 2, 2025
12 checks passed
@wprzytula wprzytula mentioned this pull request Apr 3, 2025
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.

Enable configuring local address and range of ports for client sockets to be bound to

3 participants