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+fix(gossip): update naming in active set and fix table trim condition #184

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

yewman
Copy link
Contributor

@yewman yewman commented Jun 25, 2024

Overview

  1. Rename pruned_peers to peers in ActiveSet
  2. Fix shouldTrim method in GossipTable so we trim when 90% capacity is reached, not 110%
  3. Address TODO in src/bincode/bincode.zig
  4. Update imports in modified files to fit new import style guide (draft here):

Import Style Guide (Draft)
Declare imports in the following groups, separated by a newline.

  1. group @import's
  2. group namespace aliases
  3. group struct aliases
  4. group function aliases
  5. group constant aliases

If it improves clarity, split group's into Sig and External imports / aliases.

Namespaces and Aliases
In my mind, it would be useful to establish some sort of convention for how we alias namespaces, structs, methods etc. For example in service.zig we access structs and methods from src/gossip/data.zig using a mix of approaches. Whilst this is not a big issue, in my opinion it does impact readability so is something to think about.

One approach could be to aim to import structs, methods, constants, etc into the local namespace, and only use namespace aliases when there is a conflict or it improves readability by providing context. For example, importing ContactInfo into the local namespace should never be a problem, but perhaps when using Counter from prometheus in a seperate module prometheus.Counter may be more informative.

Anyway these are just some thoughts which popped up while updating the imports!

@yewman yewman requested review from ultd, dnut and 0xNineteen June 25, 2024 19:06
Copy link
Contributor

@0xNineteen 0xNineteen left a comment

Choose a reason for hiding this comment

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

one small thing - otherwise lgtm

src/gossip/active_set.zig Show resolved Hide resolved
@yewman yewman requested a review from 0xNineteen June 25, 2024 21:27
Copy link
Contributor

@0xNineteen 0xNineteen left a comment

Choose a reason for hiding this comment

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

few small things - then should be good

src/accountsdb/snapshots.zig Show resolved Hide resolved
src/gossip/table.zig Outdated Show resolved Hide resolved
0xNineteen
0xNineteen previously approved these changes Jun 26, 2024
Copy link
Contributor

@0xNineteen 0xNineteen left a comment

Choose a reason for hiding this comment

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

lgtm - awesome first pr 🚀

@yewman yewman requested a review from 0xNineteen June 26, 2024 20:53
src/bloom/bloom.zig Outdated Show resolved Hide resolved
@0xNineteen
Copy link
Contributor

just responding to your comment - then should be good for merge

@yewman yewman requested a review from 0xNineteen June 28, 2024 15:01
Copy link
Contributor

@0xNineteen 0xNineteen left a comment

Choose a reason for hiding this comment

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

lgtm - great first pr 🔥

@0xNineteen 0xNineteen merged commit 220f002 into main Jun 28, 2024
5 checks passed
@0xNineteen 0xNineteen deleted the harnew/minor-fixes branch June 28, 2024 16:16
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.

None yet

2 participants