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

Create GossipProtocol #37

Merged
merged 1 commit into from Jul 1, 2019
Merged

Create GossipProtocol #37

merged 1 commit into from Jul 1, 2019

Conversation

MichaelKim20
Copy link
Member

@MichaelKim20 MichaelKim20 commented Jun 24, 2019

Implement class GossipProtocol
Implement class Cache
Add unittest for GossipProtocol

Copy link
Member

@TrustHenry TrustHenry left a comment

in memory cache

Copy link
Contributor

@Geod24 Geod24 left a comment

This is not connected to the node.
In order to have it connected to the node, you need to add an API endpoint in the interface API (in agora.common.API then implement it on the server side agora.node.Node).
Then add a test in agora.test.Network

source/agora/protocol/Cache.d Outdated Show resolved Hide resolved
source/agora/protocol/GossipProtocol.d Outdated Show resolved Hide resolved
source/agora/protocol/GossipProtocol.d Outdated Show resolved Hide resolved
@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Jun 24, 2019

Side note: I'd recommend you squash your commits and rebase on top of v0.x.x: https://git-scm.com/book/ko/v2/Git-%EB%B8%8C%EB%9E%9C%EC%B9%98-Rebase-%ED%95%98%EA%B8%B0

@MichaelKim20 MichaelKim20 requested a review from Geod24 Jun 24, 2019
source/agora/node/Network.d Outdated Show resolved Hide resolved
@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jun 24, 2019

The documentation mentions a "filter delegate" in code but I don't see it anywhere. Maybe the documentation needs to be updated?

@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jun 24, 2019

There are some styling inconsistencies, but I'm going to let @Geod24 decide on this because we still don't have a defined style guide (like https://dlang.org/dstyle.html).

source/agora/node/Network.d Outdated Show resolved Hide resolved
source/agora/protocol/GossipProtocol.d Outdated Show resolved Hide resolved
source/agora/test/GossipProtocol.d Outdated Show resolved Hide resolved
source/agora/test/Network.d Outdated Show resolved Hide resolved
source/agora/test/Network.d Outdated Show resolved Hide resolved
source/agora/test/Network.d Outdated Show resolved Hide resolved
source/agora/node/GossipProtocol.d Outdated Show resolved Hide resolved
@TrustHenry
Copy link
Member

@TrustHenry TrustHenry commented Jun 26, 2019

added getHasMessage of API

@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jun 27, 2019

@MukeunKim it is enough to comment only once that the pull request is updated. We will take a look.

source/agora/node/Node.d Outdated Show resolved Hide resolved
source/agora/node/Node.d Outdated Show resolved Hide resolved
@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jun 27, 2019

It looks good! I just made comments about small issues.

source/agora/test/GossipProtocol.d Outdated Show resolved Hide resolved
source/agora/node/RemoteNode.d Outdated Show resolved Hide resolved
source/agora/node/RemoteNode.d Outdated Show resolved Hide resolved
source/agora/test/GossipProtocol.d Outdated Show resolved Hide resolved
source/agora/test/GossipProtocol.d Outdated Show resolved Hide resolved
Implement class GossipProtocol
Add unittest for GossipProtocol
Geod24
Geod24 approved these changes Jul 1, 2019
/// Procedure of peer-to-peer communication
class GossipProtocol
{
private NetworkManager network;
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 1, 2019

Choose a reason for hiding this comment

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

Missing documentation for these two fields.

{
return (msg in receivedMsgCache) !is null;
}

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 1, 2019

Choose a reason for hiding this comment

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

Suggested change

@Geod24 Geod24 merged commit 64345ba into bosagora:v0.x.x Jul 1, 2019
1 check passed
@AndrejMitrovic AndrejMitrovic added the type-feature label Jul 2, 2019
@TrustHenry TrustHenry deleted the mk06 branch Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants