-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: aes-135-define-a-network-where-we-only-write-to-one-node-and-read #185
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! have a few questions/comments but nothing major 🚀
if store_in_redis { | ||
let mut conn: tokio::sync::MutexGuard<'_, MultiplexedConnection> = conn.lock().await; | ||
let stream_id_string = response.to_string(); | ||
let _: () = conn.sadd("anchor_mids", stream_id_string).await.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continuing my comment from above, if the connection is dead, we could grab user_data.redis_cli()
and make/store a new one
runner/src/simulate.rs
Outdated
peer: &Peer, | ||
stream_id: String, | ||
) -> Result<bool, anyhow::Error> { | ||
let client = reqwest::Client::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creating a new client is heavy (it has an internal threadpool). we should get one of out of the user_data object/a local static/or pass it in as a parameter.
See https://docs.rs/reqwest/latest/reqwest/struct.Client.html specifically The Client holds a connection pool internally, so it is advised that you create one and reuse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see how we can use the GooseUser outside a GooseTransaction. We can create a static client and use that. For now I can create it in simulate.rs
@@ -207,20 +237,24 @@ async fn instantiate_small_model( | |||
user: &mut GooseUser, | |||
store_in_redis: bool, | |||
conn: Arc<tokio::sync::Mutex<MultiplexedConnection>>, | |||
only_once_per_network: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only_once_per_network
confused me at first, I was thinking it was once globally, not one writer... maybe we can call it one_sided_test or one_writer_per_network or something? once makes me think literally once and that doesn't seem to be the intention.
@@ -30,6 +30,7 @@ serde_ipld_dagcbor = "0.6" | |||
serde_ipld_dagjson = "0.2" | |||
schemars.workspace = true | |||
serde_json.workspace = true | |||
once_cell = "1.19.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, just an fyi you can use a OnceLock from the std library now, but I prefer the API on once_cell tbh
4cc9317
to
df3a648
Compare
ccad158
into
feature/aes-84-validate-correctness-in-keramik-ceramic-anchoring-benchmark
Check the parent for description : #184