[HEAVILY WIP] Migrate to ClickHouse#1470
Open
skyfallwastaken wants to merge 14 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
Greptile SummaryThis PR migrates Hackatime heartbeat storage and stats reads from Postgres rollups to ClickHouse. The main changes are:
Confidence Score: 3/5This PR is not ready to merge until CI can reach ClickHouse through the native client. The migration touches core heartbeat ingest and read paths, and the CI ClickHouse service configuration currently blocks tests that exercise the new serving reader. .github/workflows/ci.yml; app/models/clickhouse/heartbeat.rb; app/models/clickhouse/heartbeat_writer.rb
 T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Rails as Rails API/Jobs
participant Writer as Clickhouse::HeartbeatWriter
participant CH as ClickHouse heartbeats
participant Delta as HeartbeatIntervals
participant Serving as Serving tables
participant Reader as Clickhouse::StatsReader
Client->>Rails: submit/query heartbeats
Rails->>Writer: insert_rows / merge / soft_delete
Writer->>CH: write heartbeat versions
Writer->>Delta: emit deltas or enqueue rebuild
Delta->>Serving: aggregate daily/user/project/dimension stats
Rails->>Reader: total_seconds/project_durations/filter_durations
Reader->>Serving: native ClickHouse queries
Serving-->>Rails: pre-aggregated stats
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant Rails as Rails API/Jobs
participant Writer as Clickhouse::HeartbeatWriter
participant CH as ClickHouse heartbeats
participant Delta as HeartbeatIntervals
participant Serving as Serving tables
participant Reader as Clickhouse::StatsReader
Client->>Rails: submit/query heartbeats
Rails->>Writer: insert_rows / merge / soft_delete
Writer->>CH: write heartbeat versions
Writer->>Delta: emit deltas or enqueue rebuild
Delta->>Serving: aggregate daily/user/project/dimension stats
Rails->>Reader: total_seconds/project_durations/filter_durations
Reader->>Serving: native ClickHouse queries
Serving-->>Rails: pre-aggregated stats
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
.github/workflows/ci.yml:155
**Expose native port**
The test job only publishes ClickHouse HTTP port `8123`, but the new `Clickhouse::ServingRepository` uses `clickhouse-native`, whose native protocol connects to TCP port `9000` by default. Any test path that reaches `StatsReader` or serving-table reads will try `localhost:9000` and fail in CI because that port is not mapped from the service container.
### Issue 2 of 2
.github/workflows/ci.yml:223
**Expose native port**
The system-test ClickHouse service has the same issue as the unit-test job: only `8123` is exposed, while `Clickhouse::ServingRepository` talks through the native client on port `9000` unless `CLICKHOUSE_NATIVE_PORT` overrides it. System tests that render project or profile stats through `StatsReader` will fail to connect to ClickHouse in CI.
Reviews (4): Last reviewed commit: "oops, remove my scratch htmls" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
Author
|
Note to self: do we need |
Member
Author
|
@greptileai review |
Member
Author
|
@greptile-apps review |
| end | ||
| end | ||
| connection.execute(<<~SQL.squish) | ||
| INSERT INTO #{table} (#{column_list(connection)}) |
| column == "user_id" ? older_user_id.to_i.to_s : connection.quote_column_name(column) | ||
| end | ||
| connection.execute(<<~SQL.squish) | ||
| INSERT INTO #{table} (#{column_list(connection)}) |
| end | ||
| end | ||
| connection.execute(<<~SQL.squish) | ||
| INSERT INTO #{table} (#{column_list(connection)}) |
| results << "user ##{newer_user.id} deleted" | ||
| end | ||
|
|
||
| Clickhouse::HeartbeatWriter.merge_user_heartbeats!(older_user_id: older_user.id, newer_user_id: newer_user.id) |
Contributor
There was a problem hiding this comment.
merge_user_heartbeats! now runs after the Postgres transaction has committed and after newer_user.destroy!. If the ClickHouse insert or tombstone fails, the newer user and related Postgres rows are already deleted while their heartbeats remain under the deleted user_id, so the account merge is only partially applied despite the rescue claiming rollback.
Artifacts
Repro: runtime version check output
- Keeps the command output available without making the summary code-heavy.
Repro: Rails boot blocker from bundle check
- Keeps the command output available without making the summary code-heavy.
Repro: executable account merge atomicity harness
- Contains supporting evidence from the run (text/x-ruby; charset=utf-8).
Repro: harness output showing partial Postgres commit after ClickHouse failure
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/admin/account_merger_controller.rb
Line: 109
Comment:
**Keep merge atomic**
`merge_user_heartbeats!` now runs after the Postgres transaction has committed and after `newer_user.destroy!`. If the ClickHouse insert or tombstone fails, the newer user and related Postgres rows are already deleted while their heartbeats remain under the deleted `user_id`, so the account merge is only partially applied despite the rescue claiming rollback.
How can I resolve this? If you propose a fix, please make it concise.
| SELECT user_id, | ||
| time, | ||
| id, | ||
| arrayJoin([#{partition_array_sql}]) AS partition_value |
| attr_reader :user_id, :reason | ||
|
|
||
| def existing_deltas? | ||
| connection.select_value("SELECT 1 FROM #{delta_table} WHERE user_id = #{user_id} LIMIT 1").present? |
| def write_initial_facts | ||
| reason_sql = connection.quote(reason) | ||
| connection.execute(<<~SQL.squish) | ||
| INSERT INTO #{delta_table} (#{INSERT_COLUMNS.join(", ")}) |
| nonzero = DELTA_COLUMNS.map { |column| "#{column} != 0" }.join(" OR ") | ||
|
|
||
| connection.execute(<<~SQL.squish) | ||
| INSERT INTO #{delta_table} (#{INSERT_COLUMNS.join(", ")}) |
Member
Author
|
@greptile-apps review |
| clickhouse: | ||
| image: clickhouse/clickhouse-server:26.6.1.1193 | ||
| ports: | ||
| - 8123:8123 |
Contributor
There was a problem hiding this comment.
The test job only publishes ClickHouse HTTP port 8123, but the new Clickhouse::ServingRepository uses clickhouse-native, whose native protocol connects to TCP port 9000 by default. Any test path that reaches StatsReader or serving-table reads will try localhost:9000 and fail in CI because that port is not mapped from the service container.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 155
Comment:
**Expose native port**
The test job only publishes ClickHouse HTTP port `8123`, but the new `Clickhouse::ServingRepository` uses `clickhouse-native`, whose native protocol connects to TCP port `9000` by default. Any test path that reaches `StatsReader` or serving-table reads will try `localhost:9000` and fail in CI because that port is not mapped from the service container.
How can I resolve this? If you propose a fix, please make it concise.| clickhouse: | ||
| image: clickhouse/clickhouse-server:26.6.1.1193 | ||
| ports: | ||
| - 8123:8123 |
Contributor
There was a problem hiding this comment.
The system-test ClickHouse service has the same issue as the unit-test job: only 8123 is exposed, while Clickhouse::ServingRepository talks through the native client on port 9000 unless CLICKHOUSE_NATIVE_PORT overrides it. System tests that render project or profile stats through StatsReader will fail to connect to ClickHouse in CI.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci.yml
Line: 223
Comment:
**Expose native port**
The system-test ClickHouse service has the same issue as the unit-test job: only `8123` is exposed, while `Clickhouse::ServingRepository` talks through the native client on port `9000` unless `CLICKHOUSE_NATIVE_PORT` overrides it. System tests that render project or profile stats through `StatsReader` will fail to connect to ClickHouse in CI.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
This PR is incomplete.
Summary of the problem
Describe your changes
Not in this PR but there's a PeerDB mirror from Postgres -> CH atm. It'll get turned off once we move
Screenshots / Media
N/A