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.

Sign up for GitHub

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

Jump to bottom

refactor(iroh-net): keep DerpMap fixed #1329

Merged
merged 5 commits into from
Aug 3, 2023
Merged

refactor(iroh-net): keep DerpMap fixed #1329

merged 5 commits into from
Aug 3, 2023

Conversation

dignifiedquire
Copy link
Contributor

Description

The DerpMap wasn't being changed anyway at runtime, only for initial config.

Closes #1182

Change checklist

  • Self-review.
  • Documentation updates if relevant.

rklaehn
rklaehn reviewed Aug 3, 2023
View reviewed changes
iroh-net/src/magicsock.rs
/// None (or zero regions/nodes) means DERP is disabled.
pub(self) derp_map: tokio::sync::RwLock<Option<DerpMap>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

🎆

rklaehn
rklaehn approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

Looks good. Less locks is more better.

But why exactly don't we need set_derp_map anymore?

@dignifiedquire
Copy link
Contributor Author

But why exactly don't we need set_derp_map anymore?

it came from the original tailscale design, but we don't actually use it, other than on startup

@dignifiedquire
Copy link
Contributor Author

Looks like some initial setup pieces are not quite working yet with this change..

@dignifiedquire dignifiedquire self-assigned this Aug 3, 2023
@dignifiedquire @divagant-martian
fix typo
683241f 
Co-authored-by: Divma <26765164+divagant-martian@users.noreply.github.com>
ramfox
ramfox approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@ramfox ramfox left a comment

Choose a reason for hiding this comment

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

I do think that in the (distant) future we will likely need something that allows us to add new derp regions to very long running nodes without restarting them, this is much simpler and all we need for now!

@dignifiedquire dignifiedquire added this pull request to the merge queue Aug 3, 2023
Merged via the queue into main with commit f764517 Aug 3, 2023
@dignifiedquire dignifiedquire deleted the fixed-derp-map branch August 3, 2023 15:46
@b5 b5 added this to the v0.6.0 - Sync milestone Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divagant-martian divagant-martian divagant-martian left review comments

@ramfox ramfox ramfox approved these changes

@rklaehn rklaehn rklaehn approved these changes

@flub flub Awaiting requested review from flub

Assignees

@dignifiedquire dignifiedquire

Labels
c-iroh-net
Projects
iroh
Archived in project
Milestone
v0.6.0-alpha.0: Sync!
Development

Successfully merging this pull request may close these issues.

Change derpmap to have Arc<DerpNode> as values
5 participants
@dignifiedquire @rklaehn @ramfox @divagant-martian @b5