Unified Storage Performance Optimizations#10
Conversation
…#97529) * dont lazy init unified storage * Inits index when creating new resource server. Fixes trace propagation by passing span ctx. Update some logging. * Use finer grained cache locking when building indexes to speed things up. Locking the whole function was slowing things down. * formatting * linter fix * go mod * make update-workspace * fix workspaces check error * update dependency owner in mod file * wait 1 second before querying metrics * try with big timeout, see if fixes CI. Wont fail locally. * skips postgres integration test. Only fails in drone. Will fix later. * put delay back to 500 ms
WalkthroughThe changes update logging and tracing practices, streamline resource server initialization, and refine concurrency control in index building. Logging is unified under internal loggers, tracing context is properly propagated, and resource server initialization is centralized. Additionally, a test is conditionally skipped based on an environment variable, and some code comments are updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceServer
participant SearchIndex
participant Watcher
Client->>ResourceServer: NewResourceServer()
ResourceServer->>ResourceServer: Init(ctx)
ResourceServer->>SearchIndex: Initialize index
ResourceServer->>Watcher: Start watcher
Client->>ResourceServer: Request (Create/Update/...)
ResourceServer->>ResourceServer: Handle request (assumes already initialized)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 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
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/storage/unified/search/bleve.go (1)
100-100: Consider expanding the TODO comment.The TODO comment could be more descriptive about what needs to be implemented.
- // TODO, check last RV so we can see if the numbers have changed + // TODO: Implement resource version checking to determine if index rebuild is necessarypkg/server/module_server_test.go (1)
35-38: Reasonable workaround for CI-specific test failures.The conditional skip for postgres in Drone CI while preserving local test functionality is a pragmatic approach. The TODO comment appropriately indicates this needs to be fixed.
Would you like me to help investigate the postgres-specific issues in the Drone CI environment or create a follow-up issue to track this technical debt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/server/module_server_test.go(1 hunks)pkg/storage/unified/resource/search.go(4 hunks)pkg/storage/unified/resource/server.go(2 hunks)pkg/storage/unified/search/bleve.go(2 hunks)pkg/storage/unified/sql/backend.go(1 hunks)
🔇 Additional comments (8)
pkg/storage/unified/search/bleve.go (1)
137-139: Excellent concurrency optimization!Moving the lock acquisition to only protect the cache assignment significantly improves concurrency by allowing multiple index builds to proceed simultaneously. This reduces contention and improves performance.
pkg/storage/unified/sql/backend.go (1)
126-126: Critical fix for tracing context propagation.Assigning the returned context from
tracer.Start()back toctxensures that the span information is properly propagated through subsequent operations in the method. This improves observability and tracing accuracy.pkg/storage/unified/resource/search.go (4)
173-173: Correct tracing context propagation.Properly assigning the returned context ensures tracing spans are correctly propagated through the initialization process.
216-216: Good addition of initialization metrics logging.The informational log provides valuable insights into search index initialization performance and document counts.
280-280: Consistent logging with internal logger.Using the internal logger instance (
s.log) instead of external loggers improves consistency and centralized log configuration.
310-310: Proper tracing context handling.Correctly assigning the context from
tracer.Start()ensures span information is available for the build operation.pkg/storage/unified/resource/server.go (2)
258-262: Excellent architectural improvement - centralized initialization.Moving server initialization to the constructor eliminates redundant initialization checks on every request and centralizes error handling. This improves both performance and maintainability by ensuring the server is fully initialized when returned from the constructor.
308-311: Logical ordering of initialization components.Starting the watcher after search initialization ensures proper dependency ordering and improves the reliability of the initialization sequence.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 10
Summary by CodeRabbit