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

Refactor batcher and inline into SessionKeeper #1036

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

LukasPukenis
Copy link
Contributor

@LukasPukenis LukasPukenis commented Dec 18, 2024

Refactor batcher and inline into SessionKeeper

This commit does these things:
- Inlines batching flag to SessionKeeper, removing batcher entirely
as a separate entity with all of its logic(threshold, traffic trigger).
- Emits all actions from SessionKeeper if batching is enabled when
an action is added. This works nicely if we can guarantee that actions
are gonna be a multiple of some T which is the case as currently we
target T=70s for proxy, direct, vpn, stun keepalives. This also makes
thresholds and the whole logic around them irrelevant.

Traffic triggered batching implementation was doing more harm than good
since it triggers on _any_ traffic, meaning even when alignment was
achieved, it might misalign since any traffic triggers it. This needs to
be done via separate channel which is out of scope for this PR

Feature flags are not removed in this commit so not to push for
version update of libtelio, so now they are no-op.

Problem

--describe problem being solved--

Solution

--describe selected solution--

☑️ Definition of Done checklist

  • Commit history is clean (requirements)
  • README.md is updated
  • Functionality is covered by unit or integration tests

@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 3678fc7 to 002eaa4 Compare December 18, 2024 09:39
@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 002eaa4 to b3979cf Compare December 18, 2024 09:45
@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from b3979cf to 5e49bb5 Compare December 18, 2024 10:02
@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 5e49bb5 to 2d34b68 Compare December 18, 2024 10:06
@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from 2d34b68 to b73a4ea Compare December 18, 2024 10:24
@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from b73a4ea to c61628a Compare December 18, 2024 11:40
@LukasPukenis LukasPukenis marked this pull request as ready for review December 18, 2024 11:41
@LukasPukenis LukasPukenis requested a review from a team as a code owner December 18, 2024 11:41
@LukasPukenis LukasPukenis force-pushed the LLT-5857_refactor_batcher branch from c61628a to 03ce6e3 Compare December 18, 2024 11:41
Copy link
Collaborator

@jjanowsk jjanowsk left a comment

Choose a reason for hiding this comment

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

❤️ ❤️ ❤️
image

Only some minor nits from my side

crates/telio-traversal/src/session_keeper.rs Outdated Show resolved Hide resolved
crates/telio-utils/src/repeated_actions.rs Outdated Show resolved Hide resolved
crates/telio-utils/src/repeated_actions.rs Show resolved Hide resolved
crates/telio-utils/src/repeated_actions.rs Show resolved Hide resolved
src/device/wg_controller.rs Outdated Show resolved Hide resolved
src/device/wg_controller.rs Outdated Show resolved Hide resolved
This commit does these things:
- Inlines batching flag to SessionKeeper, removing batcher entirely
as a separate entity with all of its logic(threshold, traffic trigger).
- Emits all actions from SessionKeeper if batching is enabled when
an action is added. This works nicely if we can guarantee that actions
are gonna be a multiple of some T which is the case as currently we
target T=70s for proxy, direct, vpn, stun keepalives. This also makes
thresholds and the whole logic around them irrelevant.

Traffic triggered batching implementation was doing more harm than good
since it triggers on _any_ traffic, meaning even when alignment was
achieved, it might misalign since any traffic triggers it.

Feature flags are not removed in this commit so not to push for
version update of libtelio, so now they are no-op.

Signed-off-by: Lukas Pukenis <[email protected]>

Signed-off-by: Lukas Pukenis <[email protected]>
Copy link
Collaborator

@jjanowsk jjanowsk left a comment

Choose a reason for hiding this comment

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

+1

Copy link

@ghztomash ghztomash left a comment

Choose a reason for hiding this comment

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

+1 looks good

src/device.rs Show resolved Hide resolved
@LukasPukenis LukasPukenis enabled auto-merge January 2, 2025 12:08
@LukasPukenis LukasPukenis merged commit f659394 into main Jan 2, 2025
64 checks passed
@LukasPukenis LukasPukenis deleted the LLT-5857_refactor_batcher branch January 2, 2025 13:14
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.

3 participants