refactor: remove unused kademlia check#555
Conversation
akrem-chabchoub
left a comment
There was a problem hiding this comment.
Thanks @mfw78 for your contribution 💯.
We should also removed this from here:
https://github.com/ethersphere/beekeeper/blob/master/config/config.yaml#L200
|
@gacevicljubisa Not sure if we are using Kademlia check elsewhere ? because AFAIK if the check isn’t included in Bee CI, it doesn’t necessarily need to be removed from Beekeeper. |
akrem-chabchoub
left a comment
There was a problem hiding this comment.
@mfw78 There are some git confilicts
Yes, it is not used in CI. But many checks are used like cron jobs on our cluster. I guess Kademlia check is not used anywhere, maybe it can be removed? @acud any opinion? |
4c5f4a6 to
9c3a626
Compare
|
Thanks for the review @akrem-chabchoub, @gacevicljubisa. I've updated the PR to address both points:
For completeness, the remaining On whether it's used elsewhere: it isn't part of Bee CI, and I don't see it referenced by any of the cluster cron checks. Happy to defer to your call on removal. |
akrem-chabchoub
left a comment
There was a problem hiding this comment.
Thanks @mfw78 💯
A couple of minor points:
- Could you open a temporary testing PR in the Bee repository that changes the Beekeeper target branch from master to your current branch? That would help verify everything works as expected
- The PR title might be more accurate as a refactor rather than a fix.
Is this strictly required? I want to avoid theatre as much as possible.
I'm ok for maintainers to modify the title of the PR as they see fit. This is just about cleaning up dead wood, and I don't want to over invest time and effort into something so trivial. |
akrem-chabchoub
left a comment
There was a problem hiding this comment.
@mfw78
I've updated the PR title.
Regarding the testing PR, I don't think it's strictly necessary in this case.
My comment was based on the general convention of validating changes that affect the Bee CI integration.
Since this is only removing unused configuration, I think we can skip it.
Happy to hear the other reviewers' thoughts.
Summary
Remove the kademlia check which evaluates internal node data structures rather than hive behavior.
Rationale
Beekeeper is for testing hives
Beekeeper's purpose is to test the behavior of multiple swarm nodes working together as a hive. The kademlia check queries individual nodes'
/topologyendpoints and evaluates internal data structures (depth, bins, population). This does not test how nodes interact with each other as a swarm.Not used in CI
The kademlia check has never been included in bee's CI workflow. The actual CI checks used are:
ci-full-connectivity,ci-settlements,ci-pss,ci-soc,ci-gsoc,ci-pushsync-chunks,ci-pushsync-light-chunks,ci-retrieval,ci-manifest,ci-feed.Hive behavior is tested elsewhere
Actual hive connectivity and routing behavior is properly tested by:
fullconnectivity- verifies nodes can communicate via ping/pongpushsync/retrieval- verifies chunks route correctly through the swarmThese checks validate how nodes work together as a hive rather than internal implementation details of individual nodes.