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

fix(sync): sleep when sync receives None and remove timeout hack #1800

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

ShahakShama
Copy link
Contributor

@ShahakShama ShahakShama commented Mar 11, 2024

This change is Reviewable

@ShahakShama ShahakShama force-pushed the shahak/remove_p2p_sync_timout_hack branch 2 times, most recently from bbc8ca1 to 436be8f Compare March 11, 2024 10:55
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

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

Project coverage is 72.12%. Comparing base (01b38b7) to head (f59b775).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/papyrus_p2p_sync/src/lib.rs 61.53% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
- Coverage   72.68%   72.12%   -0.57%     
==========================================
  Files         138      139       +1     
  Lines       19721    19925     +204     
  Branches    19721    19925     +204     
==========================================
+ Hits        14334    14370      +36     
- Misses       3230     3396     +166     
- Partials     2157     2159       +2     

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

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 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama)


crates/papyrus_p2p_sync/src/lib.rs line 41 at r1 (raw file):

                "wait_period_for_new_data",
                &self.wait_period_for_new_data.as_secs(),
                "Time in seconds to wait when a query returned with partial data before sending a \

I liked better the old phrasing. you're not sure it's returned with partial..


crates/papyrus_p2p_sync/src/lib.rs line 69 at r1 (raw file):

    // TODO(shahak): Move this error to network.
    WrongSignaturesLength { signatures: Vec<BlockSignature> },
    #[error("Network returned more responses than expected for a query.")]

add a todo here to remove this error once we can handle it


crates/papyrus_p2p_sync/src/lib.rs line 111 at r1 (raw file):

        loop {
            if matches!(control, P2PSyncControl::QueryFinishedPartially) {
                debug!(

I don't think we need this debug.. up to you


crates/papyrus_p2p_sync/src/lib.rs line 142 at r1 (raw file):

        while current_block_number.0 < end_block_number {
            let maybe_signed_header_stream_result =
                self.response_receivers.signed_headers_receiver.next().await;

what if it never joins?

@ShahakShama ShahakShama force-pushed the shahak/remove_p2p_sync_timout_hack branch from 436be8f to b1f7587 Compare March 12, 2024 08:52
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: all files reviewed, 3 unresolved discussions (waiting on @nagmo-starkware)


crates/papyrus_p2p_sync/src/lib.rs line 41 at r1 (raw file):

Previously, nagmo-starkware wrote…

I liked better the old phrasing. you're not sure it's returned with partial..

I think this relates to the last comment you wrote. If you disagree to what I wrote there let's discuss f2f


crates/papyrus_p2p_sync/src/lib.rs line 69 at r1 (raw file):

Previously, nagmo-starkware wrote…

add a todo here to remove this error once we can handle it

Done.


crates/papyrus_p2p_sync/src/lib.rs line 111 at r1 (raw file):

Previously, nagmo-starkware wrote…

I don't think we need this debug.. up to you

I think it's useful


crates/papyrus_p2p_sync/src/lib.rs line 142 at r1 (raw file):

Previously, nagmo-starkware wrote…

what if it never joins?

Then the sync will be stuck too, and that is the expected behaviour

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 all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ShahakShama)


crates/papyrus_p2p_sync/src/lib.rs line 142 at r1 (raw file):

Previously, ShahakShama wrote…

Then the sync will be stuck too, and that is the expected behaviour

as discessed f2f

@ShahakShama ShahakShama force-pushed the shahak/remove_p2p_sync_timout_hack branch from b1f7587 to f59b775 Compare March 12, 2024 09:20
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 all commit messages.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @ShahakShama)


crates/papyrus_p2p_sync/src/lib.rs line 28 at r3 (raw file):

    pub num_headers_per_query: usize,
    #[serde(deserialize_with = "deserialize_seconds_to_duration")]
    pub wait_period_for_new_data: Duration,

maybe rename to retry_sleep_duration or something like that?

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

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 4 files reviewed, all discussions resolved (waiting on @nagmo-starkware)


crates/papyrus_p2p_sync/src/lib.rs line 142 at r1 (raw file):

Previously, nagmo-starkware wrote…

as discessed f2f

Done.

@ShahakShama ShahakShama added this pull request to the merge queue Mar 12, 2024
Merged via the queue into main with commit bade592 Mar 12, 2024
16 of 17 checks passed
@ShahakShama ShahakShama deleted the shahak/remove_p2p_sync_timout_hack branch March 12, 2024 11:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 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