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

Issue #336: added retransmission of repair messages #373

Closed
wants to merge 2 commits into from

Conversation

carllin
Copy link
Contributor

@carllin carllin commented Jun 18, 2018

@carllin
Copy link
Contributor Author

carllin commented Jun 18, 2018

Link to previous broken PR: #371. Added some initial testing as per @garious requested.

@garious garious added the CI Pull Request is ready to enter CI label Jun 18, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jun 18, 2018
src/crdt.rs Outdated
num_retransmits != 0 &&
!num_retransmits.is_power_of_two()
{
return None;
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not power of two, shouldn't we transmit with id of receiver (the original logic)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, while at it, could you fix the code formatting error (causing CI failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I misunderstood this part of the original issue. Even if the goal is to reduce network congestion among the validators, it still does make more sense to just respond with a response, but not rebroadcast, on non-power of two requests. Good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done and done :)

@garious garious requested a review from aeyakovenko June 19, 2018 13:39
@garious garious added the CI Pull Request is ready to enter CI label Jun 19, 2018
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jun 19, 2018
Copy link
Member

@aeyakovenko aeyakovenko left a comment

Choose a reason for hiding this comment

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

awesome!

@garious
Copy link
Contributor

garious commented Jun 19, 2018

See #382

@garious garious closed this Jun 19, 2018
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.

5 participants