Skip to content
This repository has been archived by the owner on Dec 26, 2024. It is now read-only.

refactor(network): move responses sender into query sender #2233

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

eitanm-starkware
Copy link
Contributor

@eitanm-starkware eitanm-starkware commented Jul 17, 2024

This change is Reviewable

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 24 lines in your changes missing coverage. Please review.

Project coverage is 66.32%. Comparing base (4e3c1bd) to head (7f594d3).
Report is 2 commits behind head on main.

Files Patch % Lines
crates/papyrus_network/src/network_manager/mod.rs 69.49% 17 Missing and 1 partial ⚠️
crates/papyrus_node/src/main.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2233      +/-   ##
==========================================
+ Coverage   66.24%   66.32%   +0.08%     
==========================================
  Files         139      139              
  Lines       18343    18346       +3     
  Branches    18343    18346       +3     
==========================================
+ Hits        12151    12168      +17     
+ Misses       4895     4884      -11     
+ Partials     1297     1294       -3     

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

@eitanm-starkware eitanm-starkware force-pushed the eitan/move_result_receiver_into_query_sender branch from 750d790 to 7f594d3 Compare July 17, 2024 14:39
Copy link
Contributor

@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.

:lgtm:

Reviewed 6 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @eitanm-starkware)


crates/papyrus_network/src/network_manager/mod.rs line 45 at r2 (raw file):

    sqmr_outbound_payload_receivers: StreamHashMap<StreamProtocol, SqmrClientReceiver>,
    sqmr_outbound_response_senders: HashMap<OutboundSessionId, ResponsesSenderForNetwork>,
    sqmr_outbound_report_receivers: HashMap<OutboundSessionId, ReportReceiver>,

IMO you can unite this with inbound report receivers by changing the key type to SessionId. For now just put a TODO or you can just remember this for the server impl. Non blocking


crates/papyrus_network/src/network_manager/mod.rs line 509 at r2 (raw file):

}

fn network_send_now<Item>(

add TODO to unite those functions

@eitanm-starkware eitanm-starkware added this pull request to the merge queue Jul 18, 2024
Copy link
Contributor Author

@eitanm-starkware eitanm-starkware 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: all files reviewed, 1 unresolved discussion (waiting on @ShahakShama)


crates/papyrus_network/src/network_manager/mod.rs line 45 at r2 (raw file):

Previously, ShahakShama wrote…

IMO you can unite this with inbound report receivers by changing the key type to SessionId. For now just put a TODO or you can just remember this for the server impl. Non blocking

receivers will eventually be removed from this map and into the futureunbounded. i can combine maps and make it an option

Merged via the queue into main with commit e23be56 Jul 18, 2024
18 of 19 checks passed
@eitanm-starkware eitanm-starkware deleted the eitan/move_result_receiver_into_query_sender branch July 18, 2024 07:06
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 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