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

refactor(network): change streamed_data to streamed_bytes #1719

Merged
merged 5 commits into from
Mar 5, 2024

Conversation

ShahakShama
Copy link
Contributor

@ShahakShama ShahakShama commented Feb 19, 2024

  • refactor(network): move code around
  • refactor(network): change streamed_data to streamed_bytes

This change is Reviewable

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

Attention: Patch coverage is 65.84867% with 167 lines in your changes are missing coverage. Please review.

Project coverage is 72.72%. Comparing base (5a177ba) to head (99e00dd).

Files Patch % Lines
...yrus_network/src/converters/protobuf_conversion.rs 67.98% 39 Missing and 26 partials ⚠️
...apyrus_network/src/bin/streamed_bytes_benchmark.rs 0.00% 39 Missing ⚠️
crates/papyrus_network/src/network_manager/mod.rs 54.68% 28 Missing and 1 partial ⚠️
...tes/papyrus_network/src/streamed_bytes/messages.rs 72.60% 11 Missing and 9 partials ⚠️
...papyrus_network/src/network_manager/swarm_trait.rs 0.00% 5 Missing ⚠️
...es/papyrus_network/src/streamed_bytes/behaviour.rs 83.33% 3 Missing ⚠️
...tes/papyrus_network/src/streamed_bytes/protocol.rs 88.46% 1 Missing and 2 partials ⚠️
crates/papyrus_network/src/converters/mod.rs 96.55% 1 Missing ⚠️
crates/papyrus_network/src/lib.rs 90.00% 0 Missing and 1 partial ⚠️
...work/src/streamed_bytes/handler/inbound_session.rs 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1719      +/-   ##
==========================================
- Coverage   73.14%   72.72%   -0.42%     
==========================================
  Files         136      136              
  Lines       19446    19319     -127     
  Branches    19446    19319     -127     
==========================================
- Hits        14224    14050     -174     
- Misses       3091     3137      +46     
- Partials     2131     2132       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nagmo-starkware nagmo-starkware force-pushed the shahak/streamed_bytes_behaviour branch 2 times, most recently from b6c3bbb to 7011c5f Compare February 26, 2024 12:29
@nagmo-starkware nagmo-starkware force-pushed the shahak/streamed_bytes_behaviour branch 2 times, most recently from 0c5e6b5 to 16500a7 Compare March 4, 2024 12:31
@nagmo-starkware nagmo-starkware changed the base branch from main to nevo/network/move_to_converters-protobuf_consensusSignature_to_starknetapi_BlockSignature March 4, 2024 12:31
@nagmo-starkware nagmo-starkware force-pushed the nevo/network/move_to_converters-protobuf_consensusSignature_to_starknetapi_BlockSignature branch from 6769cac to 83e4f7a Compare March 4, 2024 13:08
@nagmo-starkware nagmo-starkware force-pushed the shahak/streamed_bytes_behaviour branch from 16500a7 to 2ea01f8 Compare March 4, 2024 13:28
Base automatically changed from nevo/network/move_to_converters-protobuf_consensusSignature_to_starknetapi_BlockSignature to main March 4, 2024 13:36
@nagmo-starkware nagmo-starkware force-pushed the shahak/streamed_bytes_behaviour branch from 2ea01f8 to 3b60eb5 Compare March 4, 2024 13:40
@nagmo-starkware nagmo-starkware force-pushed the shahak/streamed_bytes_behaviour branch from 3b60eb5 to 333e6e2 Compare March 4, 2024 14:04
Copy link
Contributor

@nagmo-starkware nagmo-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 15 files at r1, 21 of 21 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ShahakShama)


crates/papyrus_network/src/streamed_bytes/mod.rs line 66 at r2 (raw file):

}

// TODO(shahak): Consider removing protocol_name from here.

remove


crates/papyrus_network/src/streamed_bytes/mod.rs line 70 at r2 (raw file):

pub struct Config {
    pub session_timeout: Duration,
    // If we put multiple versions of the same protocol, they should be inserted sorted where the

this is an important comment. either find a way to deal with it or put it in a better place.
also consider using the Protocol enum from my PR somehow

@nagmo-starkware nagmo-starkware force-pushed the shahak/streamed_bytes_behaviour branch from 333e6e2 to 8c4e74e Compare March 5, 2024 11:48
nagmo-starkware and others added 2 commits March 5, 2024 13:54
* feat(network): convert network manager to work on bytes

* refactor(network): move conversion unctions to module

* feat(network): handle try_send error from net manager to subscriber (#1737)
* fix(network): uncomment p2p integration endpoint (#1771)

* fix(network): move file and uncomment

* fix(network): bench script works with new streamed_bytes

---------

Co-authored-by: nagmo-starkware <[email protected]>
Copy link
Contributor Author

@ShahakShama ShahakShama left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 37 files reviewed, 2 unresolved discussions (waiting on @nagmo-starkware)


crates/papyrus_network/src/streamed_bytes/mod.rs line 66 at r2 (raw file):

Previously, nagmo-starkware wrote…

remove

Done.


crates/papyrus_network/src/streamed_bytes/mod.rs line 70 at r2 (raw file):

Previously, nagmo-starkware wrote…

this is an important comment. either find a way to deal with it or put it in a better place.
also consider using the Protocol enum from my PR somehow

Done.

nagmo-starkware
nagmo-starkware previously approved these changes Mar 5, 2024
Copy link
Contributor

@nagmo-starkware nagmo-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 3 of 37 files reviewed, all discussions resolved

Copy link
Contributor

@nagmo-starkware nagmo-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 3 of 37 files reviewed, all discussions resolved

@nagmo-starkware nagmo-starkware added this pull request to the merge queue Mar 5, 2024
Merged via the queue into main with commit e942dd1 Mar 5, 2024
15 of 16 checks passed
@nagmo-starkware nagmo-starkware deleted the shahak/streamed_bytes_behaviour branch March 5, 2024 12:35
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2024
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.

2 participants