Skip to content

Conversation

DanielTavaresA
Copy link
Collaborator

Push gossip was not working well for relay because of a reference being wrongly set.
As been fixed and improved to match configuration on Go

@DanielTavaresA DanielTavaresA requested a review from a team as a code owner June 1, 2024 09:45
Copy link

github-actions bot commented Jun 1, 2024

Pull reviewers stats

Stats of the last 30 days for popstellar:

User Total reviews Time to review Total comments
K1li4nL
🥇
10
▀▀▀
4d 20h 19m
14
▀▀▀▀
simone-kalbermatter
🥈
4
8h 40m
0
onsriahi14
🥉
4
1d 23h 8m
4
matteosz
3
4d 40m
5
pierluca
2
3d 11h 3m
1
arnauds5
2
5d 15h 3m
0
DanielTavaresA
2
2d 10h 22m
8
▀▀
quadcopterman
2
6d 13h 46m
▀▀
1
Tyratox
1
44m
0
sgueissa
1
6d 11h 44m
▀▀
1
t1b00
1
18h 15m
0
osm-alt
1
25m
0
emonnin-epfl
1
51m
1
Kaz-ookid
1
1d 2h 1m
5
⚡️ Pull request stats

@DanielTavaresA DanielTavaresA changed the title [BE2} Fix gossip relay [BE2] Fix gossip relay Jun 1, 2024
Copy link

sonarqubecloud bot commented Jun 2, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Fe1-Web'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

@K1li4nL K1li4nL left a comment

Choose a reason for hiding this comment

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

Nice job ! only nitpicks comments


/** Does a step of gossipping protocol for given rpc. Tries to find a random peer that hasn't already received this msg If such a peer is found, sends message and updates table accordingly. If no peer is found, ends the protocol.
* @param rumorRpc
* Rpc that must be spreac
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Rpc that must be spreac
* Rpc that must be spread

Comment on lines 70 to 72
/** When receiving a rumor that must be relayed, empacks a rumor in a new jsonRPC and tries to do a step of gossipping protocol
* @param request
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding the second parameter in the doc ;)

}
}

/** When receives a new publish, empacks messages in a new rumor and starts gossipping it in the network
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** When receives a new publish, empacks messages in a new rumor and starts gossipping it in the network
/** When receives a new publish, packs messages in a new rumor and starts gossiping it in the network

updateGossip(newRumorRequest)
}

/** Processes a response. If a response matches a active gossip protocol, uses the reponse to decide how to continue gossipping If response is Positive (Result(0)), tries to do another step of gossipping If response is Negative (Error(-3)), considers stop gossiping
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Processes a response. If a response matches a active gossip protocol, uses the reponse to decide how to continue gossipping If response is Positive (Result(0)), tries to do another step of gossipping If response is Negative (Error(-3)), considers stop gossiping
/** Processes a response. If a response matches an active gossip protocol, uses the response to decide how to continue gosipping If response is Positive (Result(0)), tries to do another step of gosipping If response is Negative (Error(-3)), considers stop gossiping

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually double checked online and "gossipping" is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

my bad, shouldn't have blindly trusted intellij !

import akka.stream.scaladsl.Flow
import ch.epfl.pop.model.network.method.{Broadcast, Catchup, GetMessagesById}
import ch.epfl.pop.model.network._
import ch.epfl.pop.model.network.method.{Broadcast, Catchup, GetMessagesById, Rumor}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's necessary

Copy link

sonarqubecloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed for 'PoP - PoPCHA-Web-Client'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarqubecloud bot commented Jun 5, 2024

Copy link

sonarqubecloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Be1-Go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarqubecloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed for 'PoP - Fe2-Android'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@DanielTavaresA DanielTavaresA added this pull request to the merge queue Jun 5, 2024
Merged via the queue into master with commit 3e62f16 Jun 5, 2024
@DanielTavaresA DanielTavaresA deleted the fix-be2-daniel-gossip-relay branch June 5, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants