Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

adds QUIC endpoint specific for turbine connections #32294

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

behzadnouri
Copy link
Contributor

@behzadnouri behzadnouri commented Jun 27, 2023

Problem

Currently Turbine QUIC is tied to TPU implementation which makes it difficult to make changes without impacting TPU or optimize the code specifically for Turbine.

Summary of Changes

Working towards separating out Turbine QUIC from TPU, the commit adds a new QUIC endpoint implementation specific for turbine connections.
Compared to QUIC code for TPU, in this code:

  • Same endpoint sends and receives packets from the same UdpSocket.
  • Packets are sent as datagram instead of streams.
  • Connection cache can use a different eviction policy from the TPU one.

This commit only adds the new endpoint constructs; following commits will:

  • plumb through the endpoint channels to turbine code.
  • implement some cache eviction policy.
  • add metrics.

@behzadnouri behzadnouri force-pushed the turbine-quic branch 5 times, most recently from ee0fb94 to ba4d162 Compare June 27, 2023 18:31
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #32294 (ba4d162) into master (77d4632) will decrease coverage by 0.1%.
The diff coverage is 87.9%.

❗ Current head ba4d162 differs from pull request most recent head 2964e14. Consider uploading reports for the commit 2964e14 to get more accurate results

@@            Coverage Diff            @@
##           master   #32294     +/-   ##
=========================================
- Coverage    82.1%    82.0%   -0.1%     
=========================================
  Files         773      773             
  Lines      209793   209814     +21     
=========================================
- Hits       172253   172242     -11     
- Misses      37540    37572     +32     

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Looks really good! I found the whole thing really easy to read, so thanks for keeping it simple. Most of my points are small questions

let server_config = new_server_config(cert.clone(), key.clone())?;
let client_config = new_client_config(cert, key)?;
let mut endpoint = {
let _guard = runtime.enter();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this guard needed? Looking quickly into the code, I don't see that Endpoint needs to be in a runtime context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the guard, this surprisingly panics when it executes <quinn::runtime::tokio::TokioRuntime as quinn::runtime::Runtime>::wrap_udp_socket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yikes, that really stinks. Could you add a comment explaining that? It isn't immediately clear from the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

joncinque
joncinque previously approved these changes Jul 3, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Everything looks good, just the one nit to add a comment about the runtime guard. After that, this can go in

Working towards separating out turbine QUIC from TPU.
@behzadnouri
Copy link
Contributor Author

Everything looks good, just the one nit to add a comment about the runtime guard. After that, this can go in

done.

@behzadnouri behzadnouri requested a review from joncinque July 3, 2023 18:07
@behzadnouri behzadnouri added the automerge Merge this Pull Request automatically once CI passes label Jul 3, 2023
@mergify mergify bot merged commit 5a80dc0 into solana-labs:master Jul 3, 2023
gregcusack pushed a commit to gregcusack/solana that referenced this pull request Jul 5, 2023
Working towards separating out turbine QUIC from TPU.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants