Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

BroadcastProtocol introduction #5109

Merged
merged 16 commits into from
Mar 5, 2020
Merged

BroadcastProtocol introduction #5109

merged 16 commits into from
Mar 5, 2020

Conversation

jiivan
Copy link
Contributor

@jiivan jiivan commented Feb 28, 2020

No description provided.

@jiivan jiivan force-pushed the dev-version-broadcast branch 7 times, most recently from 0a80a2d to 1f91681 Compare February 28, 2020 15:41
@jiivan jiivan changed the title WIP BroadcastProtocol introduction Feb 28, 2020
@jiivan jiivan marked this pull request as ready for review March 2, 2020 09:11
@jiivan jiivan requested a review from etam March 2, 2020 09:11
Copy link
Contributor

@maaktweluit maaktweluit left a comment

Choose a reason for hiding this comment

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

LGTM!

Small comments, please fix or reply before merge

Approving to get those full tests running

golem/model.py Outdated Show resolved Hide resolved
golem/model.py Outdated Show resolved Hide resolved
golem/network/broadcast.py Show resolved Hide resolved
golem/network/broadcast.py Outdated Show resolved Hide resolved
tests/golem/network/test_broadcast.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@6f4df24). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #5109   +/-   ##
==========================================
  Coverage           ?   89.89%           
==========================================
  Files              ?      237           
  Lines              ?    22321           
  Branches           ?        0           
==========================================
  Hits               ?    20065           
  Misses             ?     2256           
  Partials           ?        0           

golem/network/broadcast.py Outdated Show resolved Hide resolved
@etam
Copy link
Contributor

etam commented Mar 4, 2020

FYI this are the current state machines:
SessionProtocol:
0_SessionProtocol
BasicProtocol:
1_BasicProtocol
ServerProtocol:
2_ServerProtocol
BroadcastProtocol:
3_BroadcastProtocol

@etam
Copy link
Contributor

etam commented Mar 4, 2020

I've cleaned up them a bit. What you think?

SessionProtocol:
0_SessionProtocol
ServerProtocol:
2_ServerProtocol
BroadcastProtocol:
3_BroadcastProtocol

EDIT: jiivan is right, that from initial and handshaking states must be a transition to disconnected, that has no callbacks.

Copy link
Contributor

@etam etam left a comment

Choose a reason for hiding this comment

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

ok

In the next step we could add an integration test.

@kmazurek
Copy link
Contributor

kmazurek commented Mar 5, 2020

@jiivan Looking through the logs of the last test run I noticed that transitions logs seem to be going to INFO level by default, e.g.:

INFO [transitions.core                   ] Exited state initial
INFO [transitions.core                   ] Entered state handshaking
INFO [transitions.core                   ] Exited state handshaking
INFO [transitions.core                   ] Entered state connected

They seem to be unnecessary for end users and will also attribute to log spam as multiple lines will be printed for each connection.

@jiivan jiivan merged commit 1ebd28f into develop Mar 5, 2020
@jiivan jiivan deleted the dev-version-broadcast branch March 5, 2020 11:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants