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

feat: add path refresher strategy #120

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Conversation

jpcsmith
Copy link
Contributor

@jpcsmith jpcsmith commented Jan 22, 2024

Add a path strategy that periodically refreshes paths.

Remaining tasks:

  • add an integration test that combines the async path strategy and path refresher.

@jpcsmith jpcsmith self-assigned this Jan 22, 2024
@jpcsmith
Copy link
Contributor Author

@mlegner Could you have a look at this when you have a moment please? It's mostly done with the exception of needed to add an integration test.

Copy link
Contributor

github-actions bot commented Jan 22, 2024

Code Coverage

Package Line Rate Health
crates/scion/src 80%
crates/scion-proto/src/packet 81%
crates/scion-proto/src/scmp 77%
crates/scion/src/daemon 91%
crates/scion/src/pan/path_strategy 83%
crates/scion-proto/src/address 70%
crates/scion-proto/src/packet/headers 85%
crates/scion-proto/src/path 83%
crates/scion-proto/src/path/standard 92%
crates/scion-proto/src/reliable 95%
crates/scion-proto/src 80%
crates/scion-proto/src/path/metadata 100%
crates/scion/src/pan 76%
Summary 80% (1809 / 2267)

@mlegner
Copy link
Contributor

mlegner commented Jan 22, 2024

Thanks for the PR, @jpcsmith, I will take a look today.

Copy link
Contributor

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this, @jpcsmith.

Everything looks good from my side. I will take a look at the tests together with the missing integration test.

crates/scion/src/pan/path_strategy/utc_instant.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/utc_instant.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
@jpcsmith jpcsmith force-pushed the feat/refreshing-strategy branch 2 times, most recently from c14cbc9 to c125c5e Compare January 22, 2024 17:32
@jpcsmith jpcsmith marked this pull request as ready for review January 22, 2024 17:33
@jpcsmith jpcsmith requested a review from mlegner January 23, 2024 12:25
Copy link
Contributor

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

Wow, I think this test suite sets a new standard for future code. 🤩

All looks good, just a few nits and minor remarks.

crates/scion/tests/test_path_strategy.rs Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
crates/scion/src/pan/path_strategy/refresher.rs Outdated Show resolved Hide resolved
@mlegner mlegner mentioned this pull request Jan 23, 2024
8 tasks
@jpcsmith jpcsmith enabled auto-merge (rebase) January 23, 2024 20:10
@jpcsmith jpcsmith merged commit 68b3197 into main Jan 23, 2024
11 checks passed
@jpcsmith jpcsmith deleted the feat/refreshing-strategy branch January 23, 2024 20:12
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