remove quic-definitions crate dependency#10013
Conversation
alexpyattaev
left a comment
alexpyattaev
left a comment
There was a problem hiding this comment.
concept r+, i wonder if we should move the notify key update into tls-utils though.
KirillLykov
force-pushed
the
klykov/no-quic-definitions-in-streamer
branch
2 times, most recently
from
cdfa65c to
d06c9b7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10013 +/- ##
=========================================
- Coverage 82.5% 82.5% -0.1%
=========================================
Files 844 844
Lines 316757 316757
=========================================
- Hits 261599 261571 -28
- Misses 55158 55186 +28 🚀 New features to boost your workflow:
|
alexpyattaev
left a comment
alexpyattaev
left a comment
There was a problem hiding this comment.
Generally looks great, a few minor questions to consider.
| tokio_util::sync::CancellationToken, | ||
| }; | ||
|
|
||
| const QUIC_KEEP_ALIVE_FOR_TESTS: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
Why do we have a different value here? If this is intentional, maybe some comment would not hurt explaining the reasoning for this choice.
There was a problem hiding this comment.
We don't have a global constant for that now. In tpu-client-next we use 1sec just because we observe that people on 3.0 and 3.1 are still using low timeout and we want to be safe. For tests, we know that the timeout is 60s and will become 10s in the follow up PR. We also know that tests are localhost so hardly we will loose the packets
| @@ -0,0 +1,9 @@ | |||
| use solana_keypair::Keypair; | |||
|
|
|||
| /// [`NotifyKeyUpdate`] is a trait used for updating the key used for QUIC connections. | |||
There was a problem hiding this comment.
| /// [`NotifyKeyUpdate`] is a trait used for updating the key used for QUIC connections. | |
| /// [`NotifyKeyUpdate`] is a trait used for updating the certificate used for QUIC connections. |
KirillLykov
force-pushed
the
klykov/no-quic-definitions-in-streamer
branch
from
d06c9b7 to
b23abbb
Compare
|
I'm catching up on old PRs -- does this mean we can completely remove solana-quic-definitions from the sdk? |
yes, but it is used in 3.1. So crate should be present but the code might be removed. |
#### Problem With anza-xyz/agave#10013, the quic-definitions crate is no longer used. Since the crate was only ever used by Agave, and isn't re-exported from the sdk anymore, it's no longer needed at all. #### Summary of changes Remove the crate and all code that references it in the repo.
|
Ok great, thanks! anza-xyz/solana-sdk#541 will remove it from the sdk |
#### Problem With anza-xyz/agave#10013, the quic-definitions crate is no longer used. Since the crate was only ever used by Agave, and isn't re-exported from the sdk anymore, it's no longer needed at all. #### Summary of changes Remove the crate and all code that references it in the repo.
4 participants
Problem
This PR removes solana-quic-definitions dependency everywhere except tpu-client-next for which it is removed in #10012
This PR doesn't modify currently used constants.
Summary of Changes