[WIP] ch cluster+replica+ha #7111
Conversation
…development start scripts
…nto uj/ch-cluster
kanwarujjaval
requested review from
Cookiezaurs,
ar2rsawseen,
Copilot and
pnrgenc
There was a problem hiding this comment.
Pull request overview
This work-in-progress PR introduces ClickHouse cluster, replication, and high availability support to Countly. The changes focus on configuration, cluster-aware mutation management, and improved development tooling.
Key changes:
- Added comprehensive ClickHouse cluster configuration with support for sharding, replication, and HA modes
- Modified mutation validation logic to handle cluster mode with
_localtable suffixes - Cleaned up extensive debug logging in API startup, converting console.log calls to structured logging
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added dev start scripts with symlink support and ClickHouse cluster test script |
| api/jobs/mutationManagerJob.js | Added cluster-aware mutation validation logic using ClusterManager to determine appropriate table names |
| api/configextender.js | Added documentation comments explaining ClickHouse configuration override mechanism |
| api/config.sample.js | Added extensive ClickHouse cluster, replication, dictionary, and identity configuration sections |
| api/api.js | Removed debug console.log statements and replaced with structured log utility calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| common.drillQueryRunner = granuralQueries; | ||
| if (common.drillDb) { | ||
| common.drillReadBatcher = new ReadBatcher(common.drillDb, {configs_db: common.db}); | ||
| console.log('✓ Drill database components initialized'); |
There was a problem hiding this comment.
Leftover console.log statement that should be converted to use the log utility like the rest of the file. This is inconsistent with the cleanup done in the rest of this file where console.log statements were replaced with log.* methods.
| console.log('✓ Drill database components initialized'); | |
| log.d('✓ Drill database components initialized'); |
| if (ClusterManager) { | ||
| try { | ||
| const cm = new ClusterManager(countlyConfig.clickhouse || {}); | ||
| if (cm.isClusterMode()) { | ||
| validationTable = task.collection + '_local'; | ||
| } | ||
| } | ||
| catch (e) { | ||
| log.w('Could not determine cluster mode for validation table', e?.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
The ClusterManager is instantiated inside a loop for every task being processed. This is inefficient and could impact performance when processing many mutation tasks. The cluster mode determination should be done once before the loop, either as a class property or by checking once at the start of the method.
| } | ||
| console.log('=== END DATABASE CONFIG CHECK ==='); | ||
|
|
||
| // Finaly set the visible title |
There was a problem hiding this comment.
Typo in comment: "Finaly" should be "Finally".
| // Finaly set the visible title | |
| // Finally set the visible title |
api/config.sample.js
Outdated
| }, | ||
| // Identity configuration for user merging and dictionary data retention | ||
| identity: { | ||
| daysOld: 30 // Number of days after which identity mappings are baked into cold partitions. |
There was a problem hiding this comment.
Missing comma after the daysOld property. This will cause a syntax error when parsing the configuration file. A comma is required after object properties in JavaScript.
| daysOld: 30 // Number of days after which identity mappings are baked into cold partitions. | |
| daysOld: 30, // Number of days after which identity mappings are baked into cold partitions. |
| if (common.drillDb) { | ||
| common.drillReadBatcher = new ReadBatcher(common.drillDb, {configs_db: common.db}); | ||
| console.log('✓ Drill database components initialized'); | ||
| common.drillReadBatcher = new ReadBatcher(common.drillDb); |
There was a problem hiding this comment.
The common.drillReadBatcher is initialized twice. Line 48 creates it with configuration options {configs_db: common.db}, but then line 50 immediately overwrites it without those options. This appears to be a merge error or leftover debug code. The second initialization should be removed, or if the configuration options are no longer needed, only the first initialization should be removed.
| common.drillReadBatcher = new ReadBatcher(common.drillDb); |
No description provided.