-
Notifications
You must be signed in to change notification settings - Fork 189
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
fix(torii/graphql): wait to query db after getting broker update #2751
Conversation
commit-id:b7448a81
WalkthroughOhayo, sensei! The changes in this pull request involve a modification to the Changes
Sequence Diagram(s)sequenceDiagram
participant Broker
participant Server
loop Event Processing
Broker->>Server: Check for new events
alt Events available
Server->>Broker: Process event
else No events
Server->>Server: Sleep for 1 second
end
end
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bin/torii/src/main.rs (1)
293-294
: Ohayo sensei! The fix looks good but could be more configurable.The addition of a delay after broker updates is a good approach to prevent excessive database queries. However, consider these improvements:
- Make the delay duration configurable through a parameter or environment variable
- Add a comment explaining why this delay is necessary
Here's a suggested improvement:
+ // Delay between broker updates to prevent excessive database queries + const BROKER_UPDATE_DELAY: Duration = Duration::from_secs(1); async fn spawn_rebuilding_graphql_server( shutdown_tx: Sender<()>, pool: Arc<SqlitePool>, external_url: Option<Url>, proxy_server: Arc<Proxy>, + broker_delay: Option<Duration>, ) { let mut broker = SimpleBroker::<Model>::subscribe(); + let delay = broker_delay.unwrap_or(BROKER_UPDATE_DELAY); loop { let shutdown_rx = shutdown_tx.subscribe(); let (new_addr, new_server) = torii_graphql::server::new(shutdown_rx, &pool, external_url.clone()).await; tokio::spawn(new_server); proxy_server.set_graphql_addr(new_addr).await; if broker.next().await.is_none() { break; } else { - tokio::time::sleep(Duration::from_secs(1)).await; + tokio::time::sleep(delay).await; } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/torii/src/main.rs
(1 hunks)
🔇 Additional comments (1)
bin/torii/src/main.rs (1)
293-294
: Verify the impact of broker updates on database performance.
Let's ensure this change effectively addresses the database query rate issue.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2751 +/- ##
==========================================
- Coverage 56.03% 56.02% -0.01%
==========================================
Files 427 427
Lines 54589 54589
==========================================
- Hits 30587 30583 -4
- Misses 24002 24006 +4 ☔ View full report in Codecov by Sentry. |
since we now use a separate
readonly
pool we need to wait for sometime for db changes to have been written before reading the db.fix: #2747
Summary by CodeRabbit