Protocol upgrade pipelining#47
Protocol upgrade pipelining#47bedeho merged 12 commits intoJoystreamClassic:developmentfrom mnaamani:protocol-upgrade-pipelining
Conversation
bedeho
left a comment
bedeho
left a comment
There was a problem hiding this comment.
I am quite happy to approve really, but perhaps we can just quickly discuss some of these issuses tomorrow.
| namespace extension { | ||
|
|
||
| std::chrono::duration<double> | ||
| calculatePieceTimeout(const double & pieceLengthBytes, |
There was a problem hiding this comment.
This should not really be a public function
| // Remove peer | ||
| std::clog << "Dropping Peer: (incompatible protocol vesrion)" << std::endl; | ||
| libtorrent::error_code ec; | ||
| drop(ec); |
There was a problem hiding this comment.
I think this is a bit strict, its not clear that we cannot hold a connection open for normal bittorrent/DHT traffic. It could also be useful for the extension user to understand how often and what peers are using incompatible protocol version. To this end, one could introduce a new enum value in BEPSupportStatus which reflects an incompatible version explicitly. For now though, this is good enough I think, so at best making an issue out of this could be interesting, and perhaps marking it with "good first issue" or help wanted.
| _session.tick(); | ||
| } | ||
|
|
||
| if(_session.mode() == protocol_session::SessionMode::buying && |
There was a problem hiding this comment.
Why is this policing not happening inside protocol_session ? Actually, even the mode guard could be inside of that same routine?
|
|
||
| std::clog << "Failed reading piece" << alert->piece << "for" << libtorrent::print_address(endPoint.address()).c_str() << std::endl; | ||
| assert(false); | ||
| std::clog << "Failed reading piece" << alert->piece << std::endl; |
There was a problem hiding this comment.
I guess in principle this could actually fail, and we should handle it more gracefully`? Why not add an issue for this, no need to get into any analysis at this point though.
| auto peerPlugin = peer(peerId); | ||
| auto endPoint = peerPlugin->endPoint(); | ||
| // Remove registeration | ||
| _outstandingLoadPieceForBuyers.erase(it); |
There was a problem hiding this comment.
Perhaps I am missing something, but are we no longer telling session about peerId of pieceread? how is that possible?
2 participants
Updates to work with new changes in protocol_session JoystreamClassic/protocol_session-cpp#27