right size s3 committer after it is caught up #298
Conversation
WalkthroughAdds a CommitterIsLive configuration flag; moves insight-service HTTP helpers into Changes
Sequence Diagram(s)sequenceDiagram
participant Poll as pollLatest loop
participant Config as Config (CommitterIsLive)
participant Libs as internal/libs
participant Service as Insight Service HTTP
Note over Poll,Config: periodic pollLatest iteration
Poll->>Config: read CommitterIsLive
alt CommitterIsLive == false and within 20 blocks and not rightsized
Poll->>Libs: RightsizeS3Committer()
activate Libs
Libs->>Service: HTTP POST /service/chains/{ChainIdStr}/rightsize-s3-committer
Service-->>Libs: 2xx / non-2xx
Libs-->>Poll: result (log)
deactivate Libs
Poll-->Poll: set hasRightsized = true
end
sequenceDiagram
participant Backfill as GetBackfillBoundaries
participant Libs as internal/libs
Backfill->>Libs: libs.DisableIndexerMaybeStartCommitter()
Libs-->>Backfill: returns / logs (may start committer)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![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
🧹 Nitpick comments (2)
internal/committer/poollatest.go (1)
78-78: Consider making the 20-block threshold configurable.The hardcoded threshold of 20 blocks may need adjustment for different chain speeds or operational requirements.
You could add a configuration field:
CommitterRightsizeThreshold int `env:"COMMITTER_RIGHTSIZE_THRESHOLD" envDefault:"20"`Then use it here:
-if !config.Cfg.CommitterIsLive && latestBlock.Int64()-int64(nextBlockNumber) < 20 { +if !config.Cfg.CommitterIsLive && latestBlock.Int64()-int64(nextBlockNumber) < int64(config.Cfg.CommitterRightsizeThreshold) {internal/libs/insightServiceRequests.go (1)
27-84: Consider returning errors for critical operations.The current fire-and-forget pattern logs errors but doesn't propagate them to callers. For critical operations like
DisableIndexerMaybeStartCommitter, the caller might need to know if the request succeeded.If you want callers to handle failures, consider:
-func makeS3CommitterRequest(endpoint string) { +func makeS3CommitterRequest(endpoint string) error { serviceURL := config.Cfg.InsightServiceUrl apiKey := config.Cfg.InsightServiceApiKey zeetDeploymentId := config.Cfg.ZeetDeploymentId // Prepare request payload requestBody := DeployS3CommitterRequest{ ZeetDeploymentId: zeetDeploymentId, } jsonData, err := json.Marshal(requestBody) if err != nil { log.Error().Err(err).Msg("Failed to marshal request body") - return + return fmt.Errorf("failed to marshal request body: %w", err) } // ... similar changes for other error paths + return nil }Then update the public functions:
-func DisableIndexerMaybeStartCommitter() { - makeS3CommitterRequest("deploy-s3-committer") +func DisableIndexerMaybeStartCommitter() error { + return makeS3CommitterRequest("deploy-s3-committer") }However, if the fire-and-forget pattern is intentional for resilience, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
configs/config.go(1 hunks)internal/backfill/getbackfillboundaries.go(1 hunks)internal/committer/poollatest.go(1 hunks)internal/committer/reorg.go(1 hunks)internal/libs/insightServiceRequests.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/committer/poollatest.go (2)
configs/config.go (1)
Cfg(84-84)internal/libs/insightServiceRequests.go (1)
RightsizeS3Committer(22-24)
internal/backfill/getbackfillboundaries.go (1)
internal/libs/insightServiceRequests.go (1)
DisableIndexerMaybeStartCommitter(18-20)
internal/libs/insightServiceRequests.go (1)
internal/libs/rpcclient.go (1)
ChainIdStr(12-12)
🔇 Additional comments (4)
internal/committer/reorg.go (1)
60-60: LGTM! Enhanced error diagnostics.Including the actual block numbers in the error message significantly improves debuggability.
configs/config.go (1)
63-63: LGTM! Configuration field properly defined.The new
CommitterIsLiveflag is correctly structured with appropriate environment variable binding and a sensible default value.internal/backfill/getbackfillboundaries.go (1)
25-25: LGTM! Correct package reference.The qualified call to
libs.DisableIndexerMaybeStartCommitter()properly reflects the function's relocation to the libs package.internal/libs/insightServiceRequests.go (1)
1-1: LGTM! Clean API refactoring.The package relocation to
libsand the new public entry points (DisableIndexerMaybeStartCommitterandRightsizeS3Committer) provide a clean, reusable interface with proper separation of concerns.Also applies to: 18-24
Summary by CodeRabbit
New Features
Behavior
Bug Fixes
Refactor