-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: allow setting Kad K_VALUE
using feat flag
#5988
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
base: master
Are you sure you want to change the base?
Conversation
af0abea
to
4fdc5dc
Compare
Amended the commit to mutex all |
0a39dec
to
78514ce
Compare
Updated the CI to run the |
Introduces new feature flags to change the `K_VALUE` for Kademlia. The new feat flags are: `k-4`, `k-8`, `k-12` and `k-16`.
78514ce
to
0a1880e
Compare
0a1880e
to
e9b7c94
Compare
Hi @mickvandijke, thank you for your PR! Citing @guillaumemichel from #5501 (comment):
Can you expand a little bit on why you need to modify 2) or 3)? Implementation wise @guillaumemichel is probably best to review this, but I don't think we should use feature-flags.
Two alternative solutions that came to my mind:
|
I agree with @elenaf9, feature flags are probably not the correct way to add configurability for
Both suggested solutions seem like a better way to proceed. |
Description
Introduces new feature flags to change the
K_VALUE
for Kademlia. The new feat flags are:k-4
,k-8
,k-12
andk-16
.Related issues:
Notes & open questions
I know that Config::set_kbucket_size is a thing, but that doesn't change the
K_VALUE
, which influences many parts of the code.I have tried making the
K_VALUE
configurable using a compile time environment variable and a build script, which would arguably be a cleaner solution, but I couldn't get it to work.So I thought using feat flags would be a nice, although maybe a bit unorthodox alternative, that doesn't require significant code refactoring.
Change checklist