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

Optimize /etc/hosts writes #259

Merged
merged 6 commits into from
Jun 2, 2023
Merged

Optimize /etc/hosts writes #259

merged 6 commits into from
Jun 2, 2023

Conversation

evaporei
Copy link
Contributor

@evaporei evaporei commented May 27, 2023 •
edited
Loading

The commit messages and issue should provide enough context for the PR 😉

Solves #233

@evaporei evaporei mentioned this pull request May 27, 2023
Open
bschwind
bschwind approved these changes May 28, 2023
View reviewed changes
Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

Overall this looks good, thanks for picking it up!

hostsfile/src/lib.rs Outdated Show resolved Hide resolved
@alerque
Copy link
Contributor

alerque commented May 28, 2023 •
edited
Loading

This will be a welcome change reducing the churn in system diff files and incremental backups. I have a few pretty "still" system setups where little or nothing changes in /etc/ for extended periods of time, but Innernet has been consistently shuffling the cheese in hosts files causing unnecessary system change tracking.

@bschwind
Copy link
Member

There's also a separate issue I want to look into, where repeated calls to innernet fetch will result in the server claiming a bunch of peers have been modified, even though it's very unlikely they actually changed. Unrelated to this PR though.

bschwind
bschwind reviewed May 28, 2023
View reviewed changes
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
strohel
strohel reviewed May 29, 2023
View reviewed changes
Copy link
Member

@strohel strohel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for picking this up! This would be a nice addition.

Functionality-wise the current version looks good (modulo one stale debug log + a suggestion to tweak the other logging). But I've added a comment that suggest doing the comparison at a slightly different level, curious what you'd think.

hostsfile/src/lib.rs Outdated Show resolved Hide resolved
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
@evaporei evaporei force-pushed the optimize-etc-hosts-writes branch from 1ef71a4 to 79e692b Compare May 30, 2023 11:50
evaporei
evaporei commented May 30, 2023
View reviewed changes
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
evaporei
evaporei commented May 30, 2023
View reviewed changes
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
strohel
strohel requested changes May 30, 2023
View reviewed changes
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
client/src/main.rs Show resolved Hide resolved
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
@evaporei evaporei force-pushed the optimize-etc-hosts-writes branch from b10685c to 52384ef Compare May 31, 2023 13:24
evaporei added 4 commits May 31, 2023 10:44
@evaporei
hostsfile: change internal map from hash to btree
f7c1a1b 
This change makes the innernet section of /etc/hosts always ordered and
deterministic. We can take advantage of that to avoid writes, that will
be done in another commit.
@evaporei
hostsfile: reduce number of writes if content hasn't changed
9ec8d00
@evaporei
hostsfile: return bool to inform if file has been written
892231d 
This commit also makes the logs print accordingly to the new behavior.
@evaporei
hostsfile: remove has_content_changed in favor of comparing old and n…
ca58977 
…ew sections
@evaporei evaporei force-pushed the optimize-etc-hosts-writes branch from 52384ef to ca58977 Compare May 31, 2023 13:44
bschwind
bschwind reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

This is looking really nice and clean, good work!

I found one more thing to change, and then I'll leave the final reviews to @mcginty and @strohel

client/src/main.rs Outdated Show resolved Hide resolved
bschwind
bschwind approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

Final LGTM from me, I'll pass it along to @mcginty and @strohel

strohel
strohel approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@strohel strohel left a comment

Choose a reason for hiding this comment

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

Perfect! Added one last nit, but approved already. Thanks for all the iterations.

Before merging I'll give this some testing in the coming days.

client/src/main.rs Show resolved Hide resolved
hostsfile/src/lib.rs Outdated Show resolved Hide resolved
@evaporei
Copy link
Contributor Author

evaporei commented Jun 1, 2023

Thank you all for the thoughtful reviews and suggestions 😊

@strohel
Copy link
Member

strohel commented Jun 2, 2023

Just quickly tested, works well for me, merging.

@strohel strohel merged commit de7ec99 into tonarino:main Jun 2, 2023
@fdkevin0 fdkevin0 mentioned this pull request Aug 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcginty mcginty mcginty left review comments

@bschwind bschwind bschwind approved these changes

@strohel strohel strohel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@evaporei @alerque @bschwind @strohel @mcginty