max memory usage #275
max memory usage #275
Conversation
|
We're building your pull request over on Zeet. |
WalkthroughAdded maxMemoryUsage support for ClickHouse across CLI flags, Viper bindings, and configuration struct. Updated query assembly to aggregate ClickHouse SETTINGS (max_execution_time, max_memory_usage) into a single clause appended at the end, only when applicable. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Flags
participant Viper as Viper Config
participant Config as Config Loader
participant CH as ClickHouse Query Builder
participant DB as ClickHouse
CLI->>Viper: Define flags (maxMemoryUsage per scope)
Viper->>Config: Unmarshal into ClickhouseConfig (MaxMemoryUsage)
Config->>CH: Provide config values
CH->>CH: Build query body
CH->>CH: addPostQueryClauses(settings: max_execution_time, max_memory_usage)
CH->>DB: Execute query with single SETTINGS clause (if any)
DB-->>CH: Results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (3)
configs/config.go (1)
88-88: Consider using uint64 for memory values to avoid overflow on 32-bit systemsWhile 1e9 fits within int, memory limits can exceed 2 GiB in some deployments. Using uint64 provides safer headroom for large byte values.
Apply this minimal change:
- MaxMemoryUsage int `mapstructure:"maxMemoryUsage"` + MaxMemoryUsage uint64 `mapstructure:"maxMemoryUsage"`internal/storage/clickhouse.go (1)
673-684: Optional: add a small unit test to lock query shape with both settings presentGiven you already expose TestQueryGeneration, a focused test will prevent regressions in clause ordering.
Example test (new file: internal/storage/clickhouse_query_test.go):
package storage import ( "math/big" "testing" config "github.com/thirdweb-dev/indexer/configs" ) func TestBuildQuery_AppendsSettingsOnce(t *testing.T) { cfg := &config.ClickhouseConfig{ Database: "db", MaxQueryTime: 30, MaxMemoryUsage: 1_000_000_000, } c, err := NewClickHouseConnector(cfg) if err != nil { t.Fatalf("connector init: %v", err) } q := c.TestQueryGeneration("blocks", "block_number", QueryFilter{ ChainId: big.NewInt(1), Limit: 10, }) want := "SETTINGS max_execution_time = 30, max_memory_usage = 1000000000" if count := len(regexp.MustCompile(`\bSETTINGS\b`).FindAllStringIndex(q, -1)); count != 1 { t.Fatalf("expected one SETTINGS clause, got %d in %q", count, q) } if !strings.Contains(q, want) { t.Fatalf("missing expected SETTINGS: %q in query: %q", want, q) } }Happy to add this in a follow-up PR if helpful.
cmd/root.go (1)
87-87: Optional: widen type to Uint64 for large memory limitsIf you anticipate values >2 GiB, prefer Uint64 flags and a uint64 field in config. This prevents overflow on 32-bit systems and communicates the domain more clearly.
Suggested changes to flag declarations:
- rootCmd.PersistentFlags().Int("storage-orchestrator-clickhouse-maxMemoryUsage", 1000000000, "Clickhouse max memory usage in bytes for orchestrator storage") + rootCmd.PersistentFlags().Uint64("storage-orchestrator-clickhouse-maxMemoryUsage", 1_000_000_000, "Clickhouse max memory usage in bytes for orchestrator storage") - rootCmd.PersistentFlags().Int("storage-main-clickhouse-maxMemoryUsage", 1000000000, "Clickhouse max memory usage in bytes for main storage") + rootCmd.PersistentFlags().Uint64("storage-main-clickhouse-maxMemoryUsage", 1_000_000_000, "Clickhouse max memory usage in bytes for main storage") - rootCmd.PersistentFlags().Int("storage-staging-clickhouse-maxMemoryUsage", 1000000000, "Clickhouse max memory usage in bytes for staging storage") + rootCmd.PersistentFlags().Uint64("storage-staging-clickhouse-maxMemoryUsage", 1_000_000_000, "Clickhouse max memory usage in bytes for staging storage")And pair with changing ClickhouseConfig.MaxMemoryUsage to uint64 as suggested in configs/config.go.
Also applies to: 109-109, 119-119, 202-202, 215-215, 228-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
cmd/root.go(6 hunks)configs/config.go(1 hunks)internal/storage/clickhouse.go(1 hunks)
🔇 Additional comments (5)
configs/config.go (1)
88-88: LGTM: Config supports ClickHouse maxMemoryUsageThe new MaxMemoryUsage field and mapstructure tag align with the new flags and Viper bindings. Unmarshalling should work as expected.
internal/storage/clickhouse.go (2)
673-684: LGTM: Aggregate ClickHouse SETTINGS and append once at the endCollecting applicable settings into a single SETTINGS clause is cleaner and avoids multiple scattered clauses. The conditional emission and comma-join are correct.
673-684: No duplicate SETTINGS clauses found
Verified that the only places emittingSETTINGSin query strings are:
- The performance‐tuning snippet at internal/storage/clickhouse.go:673–684
- The consistency query in getMaxBlockNumberConsistent at internal/storage/clickhouse.go:898
The addPostQueryClauses path does not append anySETTINGS.cmd/root.go (2)
87-87: LGTM: Flags and Viper bindings for maxMemoryUsage are consistent and correctly named
- Flags: storage-*-clickhouse-maxMemoryUsage with sensible default and clear “bytes” description.
- Bindings: storage.*.clickhouse.maxMemoryUsage keys match mapstructure tags and config field.
Also applies to: 109-109, 119-119, 202-202, 215-215, 228-228
87-87: Verify default behavior: do we want a non-zero default overriding server-side settings?Setting 1e9 by default will force a memory cap even when users don't set anything. Consider defaulting to 0 (unset) to inherit ClickHouse server defaults, unless product requirements dictate otherwise.
Would you like me to open a follow-up to switch the defaults to 0 and document the behavior?
Also applies to: 109-109, 119-119
Summary by CodeRabbit
New Features
Refactor