Add option to manually enable indexer #290
Conversation
WalkthroughThe changes introduce a new struct called Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Indexer
Client->>Server: Start Replication Server
Server->>Server: Check options.Indexer.Enable
alt If Indexer is enabled
Server->>Indexer: Initialize Indexer
Indexer-->>Server: Indexer Initialized
Server->>Indexer: Start Indexer
Indexer-->>Server: Indexer Started
else If Indexer is disabled
Server->>Server: Skip Indexer Initialization
end
Server-->>Client: Replication Server Started
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
pkg/server/server.go (1)
104-114: Document that indexer can be nil.Consider adding a comment to the
ReplicationServerstruct documenting thatindxcan be nil when the indexer is disabled. This helps prevent nil pointer dereferences in future code changes.type ReplicationServer struct { apiServer *api.ApiServer syncServer *sync.SyncServer ctx context.Context cancel context.CancelFunc log *zap.Logger registrant *registrant.Registrant nodeRegistry registry.NodeRegistry + // indx can be nil if the indexer is disabled via options.Indexer.Enable indx *indexer.Indexer options config.ServerOptions metrics *metrics.Server writerDB *sql.DB }pkg/config/options.go (1)
30-32: Consider adding a default value for clarity.The struct follows good configuration practices with clear naming and proper tags. However, consider adding a
default:"false"tag to make the default behavior explicit, consistent with other option structs in the file.type IndexerOptions struct { - Enable bool `long:"enable" env:"XMTPD_INDEXER_ENABLE" description:"Enable the indexer"` + Enable bool `long:"enable" env:"XMTPD_INDEXER_ENABLE" description:"Enable the indexer" default:"false"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
pkg/config/options.go(2 hunks)pkg/server/server.go(1 hunks)
🔇 Additional comments (3)
pkg/server/server.go (1)
104-114: LGTM! Verify indexer dependencies.
The conditional initialization of the indexer looks good. The error handling and initialization sequence are correct.
Let's verify if any code depends on the indexer being non-nil:
✅ Verification successful
Indexer usage is properly handled, no additional nil checks needed
The search results show that the indexer field (s.indx) is only used in two places within the server package:
- During initialization to start the indexer
- During shutdown to close the indexer
Both usages are properly guarded:
- The start operation only occurs within the
if options.Indexer.Enableblock - The close operation is within the
Close()method which already has a nil check (if s.indx != nil)
No other components directly depend on or assume the indexer's existence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the indexer field
rg -A 3 "\.indx\." pkg/
# Search for any indexer-related method calls
rg -A 3 "indexer\." pkg/
Length of output: 752
pkg/config/options.go (2)
108-108: LGTM! Clean integration following established patterns.
The IndexerOptions integration into ServerOptions maintains consistent structure and naming conventions with other option groups.
30-32: Verify the implementation of the indexer toggle.
Let's ensure the new option is properly handled in the server implementation.
Also applies to: 108-108
✅ Verification successful
Indexer toggle implementation is properly handled
The verification shows that:
- The indexer option is correctly checked in server initialization
- Proper error handling is in place with error propagation
- The indexer is conditionally initialized only when enabled
- All required dependencies (writerDB, contracts, validationService) are passed to the indexer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper implementation of indexer toggle
# Check the server implementation for proper handling of the indexer option
echo "Checking server implementation..."
rg -A 5 "options\.Indexer\.Enable" pkg/server/
# Check for any potential error handling related to indexer
echo "Checking error handling..."
rg -A 5 "indexer.*err" pkg/server/
Length of output: 508
Script:
#!/bin/bash
# Let's check for the complete indexer initialization and error handling context
# Get broader context around indexer initialization
echo "Checking indexer initialization context..."
rg -B 5 -A 10 "if options\.Indexer\.Enable" pkg/server/
# Check for indexer struct definition and related code
echo "Checking indexer struct and related code..."
ast-grep --pattern 'type Server struct {
$$$
indx $_
$$$
}'
# Check for error handling in StartIndexer
echo "Checking StartIndexer error handling..."
rg -B 2 -A 3 "StartIndexer" pkg/server/
Length of output: 1398
|
What is the required combo of flags? Does the indexer make sense without the sync service? Can the sync service run without it? If the answer is that both must run together or neither should be run, it should be just one flag. |
|
They can run independently, we are just likely to choose for them to be run together because it's easier to have one worker service that has a desired capacity of 1. But we could break them into two separate workers if we chose to. |
tl;dr
Summary by CodeRabbit