Deprecate explicit reuseport another way#6639
Conversation
alexpyattaev
force-pushed
the
deprecate_explicit_reuseport_another_way
branch
from
51e42cb to
ed68e61
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6639 +/- ##
=========================================
- Coverage 82.8% 82.8% -0.1%
=========================================
Files 849 849
Lines 379373 379557 +184
=========================================
+ Hits 314300 314436 +136
- Misses 65073 65121 +48 🚀 New features to boost your workflow:
|
bw-solana
left a comment
bw-solana
left a comment
There was a problem hiding this comment.
I much prefer this direction in the sense we won't be rugging anyone on the APIs.
@t-nelson - do you bless this direction?
As far as the implementation, the important stuff looks mostly correct to me.
I do see quite a few pub APIs that seem to have been removed completely from net-utils::lib (or maybe I'm just missing them). Would like to understand why they weren't deprecated instead.
|
|
||
| #[deprecated( | ||
| since = "2.3.1", | ||
| note = "Please avoid this function, it is easy to misuse" |
There was a problem hiding this comment.
should we direct people to the sockets.rs version?
There was a problem hiding this comment.
Yes, thank you for catching this!
| } | ||
|
|
||
| #[cfg(feature = "dev-context-only-utils")] | ||
| pub async fn bind_to_async( |
There was a problem hiding this comment.
its not removed, it is moved to sockets.rs and behind a feature flag where we commonly hide all sorts of unstable nonsense. Not sure if we have to follow the deprecation policy in this case.
Yeah it is probably a bit cleaner, but also more code churn (not just in the library but also at the callsites).
I've moved without proper deprecation only the functions that were behind DCOU feature, as they are our internal stuff only relevant to unittests (if someone is using those in production they deserve the trouble). If I have accidentally nuked actual pub stuff then it was not intended of course. |
bw-solana
left a comment
bw-solana
left a comment
There was a problem hiding this comment.
LGTM. Should be fine to rug the dev-context-only-utils APIs
|
Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis. |
* deprecate reuseport flag * fix tests and common usage of reuseport functions * clean up external users of .reuseport() (cherry picked from commit 0cdebbc) # Conflicts: # net-utils/src/lib.rs
* deprecate reuseport flag * fix tests and common usage of reuseport functions * clean up external users of .reuseport() (cherry picked from commit 0cdebbc) # Conflicts: # net-utils/src/lib.rs
* deprecate reuseport flag * fix tests and common usage of reuseport functions * clean up external users of .reuseport() (cherry picked from commit 0cdebbc) # Conflicts: # net-utils/src/lib.rs
3 participants
Problem
Basically #5965 could not get any progress due to API break concerns, so this is another way to look at the same problem.
Summary of Changes