refactor: replace Console loop with Swoole Timer for stats resource m…#10054
refactor: replace Console loop with Swoole Timer for stats resource m…#10054christyjacob4 merged 8 commits into1.7.xfrom
Conversation
📝 WalkthroughWalkthroughA new protected property Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Tasks/StatsResources.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Tasks/StatsResources.php (2)
7-7: LGTM! Import addition is necessary.The Swoole\Timer import is correctly added to support the new Timer::tick functionality.
63-79: Callback structure looks good.The callback structure correctly captures the
$queuevariable and maintains the existing logic for:
- Disabling authorization
- Querying projects accessed in the last 24 hours
- Queueing projects for stats resource aggregation
The internal functionality remains unchanged, which is appropriate for this refactoring.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
…timer interval in StatsResources
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Appwrite/Platform/Action.php (1)
93-97: Consider making filter names configurable for better maintainability.The hardcoded filter names make the method less flexible and harder to maintain. Consider making them configurable or at least grouping them more clearly.
- $filters = [ - 'subQueryKeys', 'subQueryWebhooks', 'subQueryPlatforms', 'subQueryProjectVariables', 'subQueryBlocks', 'subQueryDevKeys', // Project - 'subQueryAuthenticators', 'subQuerySessions', 'subQueryTokens', 'subQueryChallenges', 'subQueryMemberships', 'subQueryTargets', 'subQueryTopicTargets',// Users - 'subQueryVariables', // Sites - ]; + $filters = [ + // Project-related subqueries + 'subQueryKeys', + 'subQueryWebhooks', + 'subQueryPlatforms', + 'subQueryProjectVariables', + 'subQueryBlocks', + 'subQueryDevKeys', + // User-related subqueries + 'subQueryAuthenticators', + 'subQuerySessions', + 'subQueryTokens', + 'subQueryChallenges', + 'subQueryMemberships', + 'subQueryTargets', + 'subQueryTopicTargets', + // Site-related subqueries + 'subQueryVariables', + ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Action.php(1 hunks)src/Appwrite/Platform/Tasks/StatsResources.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Tasks/StatsResources.php
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Action.php (1)
99-109: Verify the global impact of filter registration.This method registers global database filters that will affect all database operations throughout the application lifecycle. Ensure this behavior is intended and won't cause unexpected side effects in other parts of the system.
#!/bin/bash # Description: Search for other usages of Database::addFilter to understand the impact # Expected: Find other filter registrations and verify they don't conflict echo "Searching for Database::addFilter usage..." rg "Database::addFilter" -A 3 -B 1 echo -e "\nSearching for filter usage in database operations..." rg "subQuery(Keys|Webhooks|Platforms|ProjectVariables|Blocks|DevKeys|Authenticators|Sessions|Tokens|Challenges|Memberships|Targets|TopicTargets|Variables)" -A 2 echo -e "\nSearching for StatsResources usage to understand context..." fd -e php StatsResources --exec rg "disableSubqueries" {} \;
…or disableSubqueries to use class filters
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Appwrite/Platform/Action.php (1)
100-115: Add missing documentation and return type as previously requested.The method implementation looks correct, but it still lacks proper documentation and return type specification as mentioned in the previous review.
Apply this diff to add the missing documentation and return type:
+ /** + * Disable Subqueries + * + * Registers no-op filters to disable database subqueries for performance optimization. + * Use this method when subqueries are not needed, such as during stats collection. + * + * @return void + */ - public function disableSubqueries() + public function disableSubqueries(): void
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Action.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Action.php (2)
6-6: LGTM: Required imports properly added.The new imports for
Console,DateTime, andDocumentare correctly added and used in the new methods below.Also applies to: 8-9
22-26: Well-organized filter definitions.The filters array is clearly organized with helpful comments categorizing the different subquery types. This makes it easy to understand which filters belong to which domain (Projects, Users, Sites).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
✨ Benchmark results
⚡ Benchmark Comparison
|
3 participants
Disable subqueries in the stats resources task
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist