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(p2p): have two internal tiers for sync: available and enabled #867

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

jansegre
Copy link
Member

@jansegre jansegre commented Nov 10, 2023

Motivation

This PR refactors internal code in order to make it easier to add sysctl controls for enabling and disabling specific sync versions.

Acceptance Criteria

  • Refactor ConnectionsManager so that instead of having enable_sync_v1 and enable_sync_v2 it has "available sync versions" and "enabled sync versions". A sync version being available means that it can be enabled or disabled at runtime. This distinction is useful because there are init-time requirements for making specific sync versions available and it is practical to deal with these separately from whether a specific sync version is enabled or not.
  • Make both sync-v1 and sync-v2 available by default, there's no CLI option to make them not available, CLI options only control whether they will be enabled, which can subsequently be controlled in runtime, but this will need a future PR that adds the sysctl controls.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@jansegre jansegre self-assigned this Nov 10, 2023
@jansegre jansegre force-pushed the refactor/available-sync-versions branch from 587180c to 4bbb6b7 Compare November 10, 2023 17:58
@jansegre jansegre requested a review from glevco November 10, 2023 17:59
msbrogli
msbrogli previously approved these changes Nov 10, 2023
hathor/p2p/manager.py Outdated Show resolved Hide resolved
glevco
glevco previously approved these changes Nov 10, 2023
@jansegre jansegre dismissed stale reviews from glevco and msbrogli via 830da1e November 10, 2023 20:52
@jansegre jansegre force-pushed the refactor/available-sync-versions branch from 4bbb6b7 to 830da1e Compare November 10, 2023 20:52
@jansegre jansegre marked this pull request as ready for review November 10, 2023 20:58
Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (0b32a31) 85.41% compared to head (830da1e) 85.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #867      +/-   ##
==========================================
- Coverage   85.41%   85.32%   -0.10%     
==========================================
  Files         281      281              
  Lines       22274    22306      +32     
  Branches     3375     3380       +5     
==========================================
+ Hits        19026    19032       +6     
- Misses       2581     2594      +13     
- Partials      667      680      +13     
Files Coverage Δ
hathor/builder/builder.py 92.18% <100.00%> (+0.16%) ⬆️
hathor/builder/cli_builder.py 76.07% <100.00%> (+1.07%) ⬆️
hathor/p2p/states/hello.py 76.23% <100.00%> (ø)
hathor/p2p/manager.py 70.94% <59.37%> (-1.38%) ⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jansegre jansegre merged commit d50a2be into master Nov 10, 2023
9 checks passed
@jansegre jansegre deleted the refactor/available-sync-versions branch November 10, 2023 22:23
@jansegre jansegre mentioned this pull request Nov 13, 2023
2 tasks
This was referenced Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants