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

Replace CoalescingHWConfigMixin with MultiCoalescingHWConfigMixin #365

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

jtluka
Copy link
Collaborator

@jtluka jtluka commented Mar 21, 2024

Description

The CoalescingHWConfigMixin is only capable of disabling adaptive TX/RX
setting. Typically the user disables adaptive coalescing and configures
specific values of rx/tx-usecs and others. Not setting these would
result in non-deterministic behaviour of the setup.

The MultiCoalescingHWConfigMixin is capable of setting any coalescing
options, so using it instead of CoalescingHWConfigMixin makes sense.

Tests

Test internally in generic_team_tracker/-/issues/829

Reviews

@olichtne

@jtluka jtluka requested a review from olichtne March 21, 2024 11:07
@olichtne
Copy link
Collaborator

so i guess the main question now is, is the CoalescingHWConfigMixin used anywhere and does it have any actual use? maybe we should simply remove it at this point and rename the MultiCoalescingHWConfigMixin into just CoalescingHWConfigMixin as it supersedes all of the functionality and we "know" that the old implementation leads to unpredictable results which i think we never want?

@olichtne
Copy link
Collaborator

and while we're at it, the MultiCoalescingMixin should probably include some documentation of the dictionary format for the coalescing_settings parameter... some example or something?

@jtluka
Copy link
Collaborator Author

jtluka commented Mar 22, 2024

so i guess the main question now is, is the CoalescingHWConfigMixin used anywhere and does it have any actual use? maybe we should simply remove it at this point and rename the MultiCoalescingHWConfigMixin into just CoalescingHWConfigMixin as it supersedes all of the functionality and we "know" that the old implementation leads to unpredictable results which i think we never want?

The mixin is currently used internally in ShortLivedConnectionsRecipe only with "adaptive_tx_coalescing": true and "adaptive_rx_coalescing": true. All of the production ready tests are in old lab that will be decomissioned, so imo we still could replace the mixin as you suggested as the impact would be minimal.

@jtluka
Copy link
Collaborator Author

jtluka commented Mar 22, 2024

and while we're at it, the MultiCoalescingMixin should probably include some documentation of the dictionary format for the coalescing_settings parameter... some example or something?

Good idea, I'll include it in this MR.

@jtluka jtluka closed this Mar 22, 2024
@jtluka jtluka reopened this Mar 22, 2024
…ngHWConfigMixin

The CoalescingHWConfigMixin is only capable of disabling adaptive TX/RX
setting. Typically the user disables adaptive coalescing and configures
specific values of rx/tx-usecs and others. Not setting these would
result in non-deterministic behaviour of the setup.

The MultiCoalescingHWConfigMixin is capable of setting any coalescing
options, so using it instead of CoalescingHWConfigMixin makes sense.

Signed-off-by: Jan Tluka <[email protected]>
@jtluka jtluka force-pushed the deprecate-coalescing-mixin branch from cc5f75b to b0622f9 Compare March 22, 2024 11:01
@olichtne olichtne merged commit 5d430e2 into LNST-project:master Mar 25, 2024
3 checks passed
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.

2 participants