fix: tcp updates + initial zmq #176
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ptarasiewiczNV
approved these changes
Feb 13, 2025
| //! Logging can take two forms: `READABLE` or `JSONL`. The default is `READABLE`. `JSONL` | ||
| //! can be enabled by setting the `TRD_LOGGING_JSONL` environment variable to `1`. | ||
| //! | ||
| //! Filters can be configured using the `TRD_LOG` environment variable or by setting the `filters` |
There was a problem hiding this comment.
Suggested change
| //! Filters can be configured using the `TRD_LOG` environment variable or by setting the `filters` | |
| //! Filters can be configured using the `TRD_LOG` environment variable or by setting the `log_filters` |
| use tracing_subscriber::{filter::Directive, fmt}; | ||
|
|
||
| /// ENV used to set the log level | ||
| const FILTER_ENV: &str = "TRD_LOG"; |
There was a problem hiding this comment.
Suggestion: change to more explicit TRD_LOGGING_LEVEL? Also following convention of TRT_LOGGING_ prefix for log config
There was a problem hiding this comment.
TRD_LOG=info is all you need.
but you can also,
TRD_LOG=triton_distributed=trace,error
which lets all trd to trace and third party bits to error
There was a problem hiding this comment.
normally it's RUST_LOG, but we can change the variable name.
rmccorm4
reviewed
Feb 13, 2025
| @@ -134,11 +133,11 @@ impl TcpClient { | |||
| .map_err(|e| error!("failed to send handshake: {:?}", e))?; | |||
|
|
|||
| // set up the channel to send bytes to the transport layer | |||
| let (bytes_tx, bytes_rx) = tokio::sync::mpsc::channel(16); | |||
| let (bytes_tx, bytes_rx) = tokio::sync::mpsc::channel(64); | |||
There was a problem hiding this comment.
How is this value tuned? When is 64 better than 16? When is 128 > 64? What's the con to increasing it? Should it be configured differently for different scenarios?
| @@ -461,133 +444,171 @@ async fn tcp_listener( | |||
| } | |||
|
|
|||
| // we need to know the buffer size from the registration options; add this to the RequestRecvConnection object | |||
| let (tx, rx) = mpsc::channel(16); | |||
| let (response_tx, response_rx) = mpsc::channel(64); | |||
| let state = server.state.clone(); | ||
|
|
||
| let id = "test-request".to_string(); | ||
| let (tx, mut rx) = tokio::sync::mpsc::channel(512); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the PR do?
Note - I am using
panic!for an unrecoverable condition. These are conditions that should never happen and should be caught in CI and unit tests.We should further address these via #171.
soak test can now easily handle 300k tcp open/closes in 1m
Checklist
<commit_type>: <Title>Commit Type:
Check the conventional commit type
box here and add the label to the github PR.
Related PRs:
Where should the reviewer start?
Test plan:
Caveats:
Background
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)