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

Improve multicast in non-ideal cases with connectivity with some of peers #2161

Merged
merged 1 commit into from Jul 29, 2020

Conversation

sandipbhoir
Copy link
Contributor

@sandipbhoir sandipbhoir commented Jul 27, 2020

Description

https://github.com/Zilliqa/Issues/issues/647

Backward Compatibility

  • This is not a breaking change
  • This is a breaking change

Review Suggestion

Status

Implementation

  • ready for review

Integration Test (Core Team)

  • local machine test
  • small-scale cloud test

@sandipbhoir sandipbhoir self-assigned this Jul 27, 2020
@sandipbhoir sandipbhoir added this to PRs in development in Core via automation Jul 27, 2020
@sandipbhoir sandipbhoir added Ready Ready for review and removed Testing_InProgress labels Jul 28, 2020
@sandipbhoir sandipbhoir moved this from PRs in development to PRs needing review in Core Jul 28, 2020
m_startbyte, m_hash));

// Park the thread if message sending taking more than expected
auto status = hThread->wait_for(std::chrono::seconds(5));
Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, messages that go beyond a few MB can potentially take (maybe) more than 5 seconds. Maybe we can add this duration to constants.xml instead so we can adjust easily if necessary.

auto status = hThread->wait_for(std::chrono::seconds(5));

if (status == std::future_status::timeout) {
LOG_GENERAL(WARNING, "Sending message to "
Copy link
Contributor

Choose a reason for hiding this comment

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

In the interest of brevity, I suggest to shorten to: "Sending delayed for "
Anyway the presence of the LOG_MARKER will already help us find the scope for this entire execution in the logs.

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. but i still prefer the message logging for peer, ofcourse short one like you suggested.

Core automation moved this from PRs needing review to PRs approved - ready to merge! Jul 28, 2020
Copy link
Contributor

@chetan-zilliqa chetan-zilliqa left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #2161 into master will decrease coverage by 0.00%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master    #2161      +/-   ##
==========================================
- Coverage   28.50%   28.50%   -0.01%     
==========================================
  Files         275      275              
  Lines       37288    37296       +8     
==========================================
+ Hits        10629    10631       +2     
- Misses      26659    26665       +6     
Impacted Files Coverage Δ
src/common/Constants.h 100.00% <ø> (ø)
src/libNetwork/P2PComm.cpp 4.90% <0.00%> (-0.07%) ⬇️
src/common/Constants.cpp 99.78% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2dbec6...3d5d35f. Read the comment docs.

@ansnunez ansnunez merged commit dd24fbf into master Jul 29, 2020
Core automation moved this from PRs approved - ready to merge! to PRs done (merged) Jul 29, 2020
@ansnunez ansnunez deleted the fix/multicast branch July 29, 2020 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready Ready for review
Projects
Core
  
PRs done (merged)
Development

Successfully merging this pull request may close these issues.

None yet

4 participants