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 QUIC new connection handler function #26855

Merged
merged 3 commits into from
Aug 5, 2022

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented Jul 29, 2022

Problem

setup_connection function has gotten convoluted over time, and could use a refactor.

Summary of Changes

Refactored the function into smaller blocks and also improve the code reuse.

Fixes #

@pgarg66 pgarg66 force-pushed the cleanup-quic-streamer branch from 0103c74 to 06b6e0f Compare July 30, 2022 00:36
@pgarg66 pgarg66 marked this pull request as ready for review July 30, 2022 00:56
@pgarg66 pgarg66 requested review from jstarry and sakridge July 30, 2022 00:56
@KirillLykov
Copy link
Contributor

Generally, it is a good idea to simplify original code by extracting functions.
Yet the code is quite sensitive and complicated while github diff doesn't show changes well.
It would help if you split this PR into 2-3 smaller commits to see clearly which part of the code goes to which function.

@pgarg66 pgarg66 force-pushed the cleanup-quic-streamer branch from 06b6e0f to 6366cde Compare August 5, 2022 12:59
@pgarg66
Copy link
Contributor Author

pgarg66 commented Aug 5, 2022

Generally, it is a good idea to simplify original code by extracting functions. Yet the code is quite sensitive and complicated while github diff doesn't show changes well. It would help if you split this PR into 2-3 smaller commits to see clearly which part of the code goes to which function.

@KirillLykov, thanks for reviewing this PR. I have tried to functionally split the PR into three commits. Hopefully it makes it easier to view the code diff. The first two commits contain the most relevant diffs.

Copy link
Contributor

@KirillLykov KirillLykov left a comment

Choose a reason for hiding this comment

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

I looked through it and didn't found any suspicious changes. Yet I would ask contributors to this part of code to check it as well


let remote_addr = connection.remote_address();
let remote_addr = new_connection.connection.remote_address();
let mut remote_pubkey = None;

let table_and_stake = {
Copy link
Contributor

Choose a reason for hiding this comment

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

A note: looks like table_and_stake is not used and will be removed in the next commits

@@ -217,6 +217,94 @@ impl NewConnectionHandlerParams {
}
}

fn handle_and_cache_new_connection(
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this commit just moves functions a bit up in the code, no changes

Copy link
Member

@sakridge sakridge left a comment

Choose a reason for hiding this comment

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

lgtm

@pgarg66 pgarg66 merged commit 3c15d26 into solana-labs:master Aug 5, 2022
@pgarg66 pgarg66 deleted the cleanup-quic-streamer branch August 5, 2022 18:24
apfitzge pushed a commit to apfitzge/agave that referenced this pull request Aug 9, 2022
* Refactor QUIC new connection handler function

* cleanup setup_connection

* more cleanup
xiangzhu70 pushed a commit to xiangzhu70/solana that referenced this pull request Aug 17, 2022
* Refactor QUIC new connection handler function

* cleanup setup_connection

* more cleanup
lijunwangs pushed a commit to lijunwangs/solana that referenced this pull request Oct 26, 2022
* Refactor QUIC new connection handler function

* cleanup setup_connection

* more cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants